search

samg / timetrap

Simple command line timetracker
http://rubygems.org/gems/timetrap
Other
1.48k stars 116 forks source link

Update the resume action to allow resuming by id, removes some old functionality #116

Closed marcaddeo closed 8 years ago

marcaddeo commented 8 years ago

This changes the resume action to allow you to either resume by an id, or resume the last checked out entry.

This removes the functionality that allowed you to change the note on the last entry, or start a new entry with the new note. IMO the former should just be accomplished by using the edit action, and the latter with the in action. I don't think there's any need to duplicate the functionality of those actions within the resume action.

Addresses #114

@samg definitely want to open a discussion on this change, as I'm removing some functionality of the resume action. Let me know your thoughts!

samg commented 8 years ago

Thanks! I'll take a look it over and provide some feedback asap.

On Thu, Oct 15, 2015 at 7:22 PM, Marc Addeo notifications@github.com wrote:

This changes the resume action to allow you to either resume by an id, or resume the last checked out entry.

This removes the functionality that allowed you to change the note on the last entry, or start a new entry with the new note. IMO the former should just be accomplished by using the edit action, and the latter with the in action. I don't think there's any need to duplicate the functionality of those actions within the resume action.

Addresses #114 https://github.com/samg/timetrap/issues/114

@samg https://github.com/samg definitely want to open a discussion on this change, as I'm removing some functionality of the resume action. Let

me know your thoughts!

You can view, comment on, or merge this pull request online at:

https://github.com/samg/timetrap/pull/116 Commit Summary

  • Update the resume action to allow resuming by id, removes some old functionality

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/samg/timetrap/pull/116.

samg commented 8 years ago

Looks good to me. Thanks!

samg commented 8 years ago

Pushed these changes out in https://rubygems.org/gems/timetrap/versions/1.9.0.

marcaddeo commented 8 years ago

Awesome! Glad to be able to start using this :)

hansdez commented 8 years ago

Unfortunately this commit made timetrap much slower to use for me and does something that is not very intuitive.

My use case is as follows: I have different sheets for different types of activities like looking for funding or writing blogpost. I use the note field to show what type of funding I am looking for or which blogpost I am writing on. Quite often I check out of the sheet, work on something else and switch back to the sheet and want to resume on the same topic (with the same note).

Before this commit I could switch back and check in with two commands like this:

t -
t r

I can easily string these commands together into a single bash alias. The resume action would find the latest checked out entry within the sheet and continue that.

After the commit I have to do this:

t -
t d -v

Visually find the latest id and then:

t r -i <latest id>

There is no way for me to automate that and make it easier. Also if I ommit the -i parameter it will find the last checked out entry in any sheet and resuming that entry. Quite a few times I have made as mistake so that a note used with a different sheet is suddenly used in the active sheet.

@samg was this intentional or can we consider it a regression? I think adding the option to resume by id makes sense, but it shouldn't interfere with the ability to just quickly resume the last entry within a particular sheet right?

samg commented 8 years ago

@hansdez this does seem to me like a regression. Seems that resume should default to the last entry on the current sheet, since otherwise we end up with the counterintuitive pulling entries across sheets which seems like it's usually the wrong thing.

@marcaddeo let me know if you disagree, or think the current behavior is more desirable for other use cases.

I'd also be easy to make this configurable, though that seems like it'd probably be better to avoid that.

hansdez commented 8 years ago

Thanks for the quick reply @samg! Let's see what @marcaddeo says. BTW: I really appreciate timetrap!!

samg commented 8 years ago

https://rubygems.org/gems/timetrap/versions/1.12.0 has a fix for this.

marcaddeo commented 8 years ago

Hey guys! Sorry I didn't respond sooner, been quite the busy couple of weeks! I definitely think this was a regression on my part... my bad!

hansdez commented 8 years ago

I've tried the 1.12.0 release and the regression is fixed for me. Thanks for all the effort!