Add editor feature

[patrickdavey]

Hi Sam,

I've created the basic functionality to use the system editor. I have tested this on my OSX box using vim & emacs & nano.

I haven't added tests yet as I'm not really sure how best to mock out the use of an external editor. I could just call out to a method which will create the tempfile etc. and then stub it in tests. Happy to implement that / whatever else you like.

Anyway, let me know if this is what you're after, and thanks for a great gem.

Fixes #101

p.s. apologies for opening & closing the other PR. I wanted to fix up my use of the IO.read (and test in a few of the editors, as someone had suggested the behaviour might be different in something other than vim)

[samg]

This looks great. Thanks for the contribution! I'm traveling at the moment but will work on getting this merged and released as soon as possible.

It would be great to add some tests to ensure this isn't broken by future changes. Maybe it'd be reasonable to stub the call to system or to wrap the whole "invoke external editor" code in a method which could be stubbed in tests to ensure that writing to the tempfile will result in desired behavior.

[patrickdavey]

Hi Sam,

Great, I'll separate out the call to the system method so that it can be stubbed easily. One enhancement to make might be the ability to edit notes in the system editor too?

One thing I haven't tested yet is non-terminal based editors (like sublime, atom etc.). One person I was chatting to suggested that those editors won't block the call to system, which would mean the tempfile won't have been written to by the time the call returns. Do you see that as a blocker? I might have a play with using sublime as the git commit program and see how it behaves. Anyway, keen to hear your thoughts on that!

[patrickdavey]

I just did a quick test (thanks this stackoverflow article)

Using sublime text as the editor does work, but it requires adding a -w flag to make the system "wait" until the editor has been exited (see the above link). It's the same thing for atom, and I assume any other non-terminal based editor

So, I think the best thing is instead of using the $EDITOR system variable, to do the same thing as git does, and have a core.editor variable which you set with the required flags.

I'll proceed with adding tests and altering the behaviour to use a core_editor setting.. but very keen to hear any comments you have if you think there's a better way to do things :)

[patrickdavey]

Hi Sam,

I've added in a spec for testing that the external editor is invoked (stubbed). I also added in a metadata helper support method as I was having issues trying to see what was happening when I was writing the tests!

At the moment this only works when you're checking "in" the note. If you want the ability to edit a timesheet entries note then I could do that as a separate PR. I looked at the editing code and there's quite a bit going on with various flags, appending etc. so I wasn't quite sure what the correct behaviour would look like. Perhaps if unused_arguments.empty? and the note_editor is set then pop the current note into an external editor?

I think this can be merged as-is, but happy to combine editing functionality if needed.

[samg]

This looks pretty good to me as is.

In terms of edit I think it would be nice to add comparable functionality there (it kind of sucks to be able to use your favorite editor for in but not for edit.) I agree it's a bit tricky to determine the right behavior there, since edit has a lot more uses than in. Maybe it would make sense to require a flag to invoke the editor when using edit, though -e is taken for "end time" so I'm not sure what would make the most sense there.

Anyway I don't think we should let complexity of potential edit improvement block this improvement. Let me know if you have ideas there. Regardless I'll merge this and push out a new gem version in the next few days.

[patrickdavey]

Hi Sam,

I agree, the edit-in-editor functionality should also be there for this to be useful.

For the "in" it only opens in an editor if you don't supply a message. I would think similar logic would work for edit. Assuming the note_editor variable is set then how about:

1. If you run t edit it would open the current note in an external editor
2. If you run t edit "my updated note" it would just set the note and not use the external editor
3. If you run t edit -z it would open the current note in an external editor with the separator appended
4. If you run t edit -z "my appended text" it would append the text and not use the external editor

I would guess most people would use the basic t edit to launch an editor.

Sound like an approach? No extra flag needed.

[samg]

That makes sense. The only cases not covered by your examples are changing start/end times and moving an entry to another sheets. Seems like the logical behavior would be to not open an editor.

t e --start "30 minutes ago" #updates start time, doesn't open editor
t e --move "another sheet" #changes sheet name, doesn't open editor

Do you agree that's the best behavior?

[patrickdavey]

Well, it's your gem so whatever you think best ;)

But yes, I think it's reasonable that you'd probably want to edit the message only on appending or with the empty t edit command.

If you're happy with the above then I'll try get it implemented in the next week or so, if you want to hold off on the release until all the functionality is there - up to you :)

[samg]

Sounds good. If you're planning to implement the edit functionality I'll hold off shipping a new gem version for a week or two.

[patrickdavey]

Yup I'm away hiking for the next 4 days, but I'll get it done after that. Well, pending internet which so far is very reliable in Colombia! :)

On Wed, 4 May 2016 01:06 Sam Goldstein notifications@github.com wrote:

Sounds good. If you're planning to implement the edit functionality I'll hold off shipping a new gem version for a week or two.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/samg/timetrap/pull/131#issuecomment-216752891

http://blog.psdavey.com