Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding hook feature #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Adding hook feature #40

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 10, 2015

to execute external scripts when special events occur.

It can be use to execute pandoc or asciidoc after editing a note for example.
This first version has only post hook script which is executed after the editor closing

occure.

This first version has only post hook script which is executed after the
editor closing
@seanh
Copy link
Owner

seanh commented Feb 12, 2015

Hi @intru91 , this sounds awesome. A hook feature would also be good for calling a vcs like git to version your notes. I'll get round to merging this asap (at the weekend, or next week)

@@ -60,6 +61,11 @@ the default default will be used"""
help="the filename extensions to recognize in the notes dir, a "
"comma-separated list (default: %(default)s)")

parser.add_argument("--hooks", dest="hooks", action="store",
default=defaults.get("hooks", "{}"),
help="the scripts which get executed when events occure such has \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typos:

occure -> occur
such has -> such as

Also I don't think the \ is necessary because Python has implied line continuation inside parentheses, so you can do:

parser.add_argument(...
    help="the scripts which get executed when events occur such as "
         "saving notes (default: %(default)s)")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get an error on python 2.7.9 when trying:
print("test test")

... Sorry can't get markdown to work but both test are on different line

@seanh
Copy link
Owner

seanh commented Feb 18, 2015

Thanks @intru91, I added a couple of small things that I'd like you to fix.

There's also a couple of slightly bigger things:

Would you mind changing the approach slightly? Rather than a --hooks argument that takes JSON, I'd prefer a --post-hook argument that just takes a simple command to execute as argument (the command could be a path to a script file or just a command). This avoids bringing JSON into it, so keeps it simpler. When/if we add more hooks, we'll add more --*-hook arguments.

I think it'd also be nice to add some documentation (in the help text for the --post-hook argument maybe) explaining what the post hook is, i.e. that it gets executed after leaving the editor and returning to tv (and it looks like it always gets executed, even if the file wasn't edited, which is fine but worth a mention in the help text).

@ghost
Copy link
Author

ghost commented Feb 19, 2015

Hi @seanh, thank you for the feedback !

I'll fix the typos.

For if hook in self.hooks != "", nice catch, it was a old code i left hanging ... . I'll replace it by if hook in self.hooks and ...

As for the user interface, JSON was a convenient way of getting data in the internal of terminal_velocity but was really bad for the user (hard to get right from the shell and exposing the internal data structures....). I actually intended to change the interface to a similar one that you described. I'll work on it and give you a feedback when i'll have something satisfying.

@seanh
Copy link
Owner

seanh commented Feb 19, 2015

Thanks! Looking forward to it

Lambert Laurent added 2 commits February 23, 2015 16:27
    For example post hook is now --post-hook on the command line.
    Internally, hooks are still represented as a dict.
@ghost
Copy link
Author

ghost commented Mar 12, 2015

Hi @seanh,

I'm pushed two new commit on my branch to apply the changes that you suggested.

@seanh
Copy link
Owner

seanh commented Mar 14, 2015

@intru91 This looks great. It has merge conflicts with the current master branch. Can you either pull master into your branch or rebase your branch on master (whichever you prefer) and solve the merge conflicts?

Conflicts:
	bin/terminal_velocity
	terminal_velocity/urwid_ui.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant