-
-
Notifications
You must be signed in to change notification settings - Fork 241
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
feat: add messages to time logs #337
Conversation
* Made 'watson.frames.Frames' a subclass of 'collections.OrderedDict'. * Changed order of items in 'watson.frames.Frame' to be more consistent with 'Frames.add()' and 'Frames.new_frame()'. This changes the on-disk JSON representation of the frames, i.e. the format of the '<app dir>/watson/frames' file. The code handles loading the old format automatically and on saving the file gets written in the new format. * Removed ability to look up frame by index with dict look-up syntax and provide 'Frames.get_by_index()' method as replacement. * Removed ability to delete frame by index (was not used aywhere). * Removed ability to get Frames column values with dict look-up syntax and provide 'Frames.get_column()' generator as replacement. * Iterating over 'Watson.frames' yields frame IDs instead of 'Frame' instances. Where the latter is desired, the 'values()' method must be used. * Adding a frame by assigning a 'Frame' instance to a key of a `Frames`, makes a copy of the 'Frame' instance with the given key as the frame ID. * Made 'Frame.filter()' a generator. * Adapted code using 'Frame' and 'Frames' to these changes. * Adapted tests and test data files. + Command line interface remains unchanged.
…ame object Signed-off-by: Christopher Arndt <[email protected]>
…mes format too; Clarify docstring on passing frames to construcutor as well Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
…der for better perfomance in most likely case Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
Signed-off-by: Christopher Arndt <[email protected]>
…evelopment)" This reverts commit c7c3c84.
…verse order for better perfomance in most likely case" This reverts commit 8d2206f.
This reverts commit c956089.
This reverts commit 73c2593.
…ist items" This reverts commit 714b581.
… new Frames format too; Clarify docstring on passing frames to construcutor as well" This reverts commit 553e34a.
…to the same object" This reverts commit 8ed29c8.
Signed-off-by: Christopher Arndt <[email protected]> Merge branch 'feature/log-message' of github.com:SpotlightKid/Watson into feature/log-message Signed-off-by: Christopher Arndt <[email protected]>
…ex'" This reverts commit a1a905a.
This reverts commit 92b8733.
…_log_message # Conflicts: # tests/test_watson.py # watson.completion # watson/cli.py
@k4nar @jnsebgosselin I could use a second opinion: I had been using -m/--message on start/stop, mirroring the git parameter of the same name. I added options --messages --no-messages to log, aggregate, and report to control whether or not messages get displayed. They really add a lot of noise, but are also useful in some cases, so I want to make sure users have can show and hide them when needed. The problem is that aggregate and report already define -m to be --month, so I could define --message, but not -m. I want to keep the message flag consistent across commands to make it more intuitive to use. However -l (L) is already taken (--log-message), -s is already taken by --csv (--string, --summary), -t is already taken by --to (--text).... I just thought of --note. No commands are using -n yet. TL;DR What do you think about renaming -m/--message to -n/--note so that we can use -n on start, stop, log, aggregate, and report? No, it's not the same as git's option, but I think a consistent abbreviation will make watson much easier to use, and the usage of -n is slightly different than git's -m anyway. |
Update: Messages now get aggregated and displayed in report and aggregate, hidden by default. |
@jnsebgosselin I'm going to go ahead with renaming -m and --message to -n and --note |
-m and -M are currently used by some commands. -n and -N aren't. This diverges from the git -m/--message naming we were copying, but that doesn't bother me too much. The parameter behaves slightly different in watson, so it might even be better that we don't reuse a commonly used parameter name in another tool, as not to cause confusion
note to self: there is a bug in watson aggregate --note. It is expecting a parameter. It doesn't function as a flag. |
@k4nar This is ready for review. |
@k4nar @jmaupetit It has been two months since I have heard from either of you on this request. It is ready to merge. What needs to happen next? |
@prat0088 : Sorry, I'll try to do a final review soon 🙏 . |
@@ -227,6 +236,7 @@ def start(ctx, watson, confirm_new_project, confirm_new_tag, args, gap_=True): | |||
|
|||
if project and watson.is_started and not gap_: | |||
current = watson.current | |||
# TODO: log in error note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop the TODO, don't you think?
@@ -485,11 +516,13 @@ def status(watson, project, tags, elapsed): | |||
help="Format output in plain text (default)") | |||
@click.option('-g/-G', '--pager/--no-pager', 'pager', default=None, | |||
help="(Don't) view output through a pager.") | |||
@click.option('-n', '--note', 'show_notes', default=False, is_flag=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be --notes
@@ -288,9 +291,14 @@ def stop(self, stop_at=None): | |||
if stop_at > arrow.now(): | |||
raise WatsonError('Task cannot end in the future.') | |||
|
|||
if note is None: | |||
note = old.get('note') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that means that if a not is passed with watson stop
, it will override a previous note passed via watson start
? Shouldn't we try to concatenate them somehow, or at least not silently drop it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure what people would prefer: concatenating or dropping. Either way silently dropping is bad I agree.
How about printing a message that the old note from start is being overwritten, print it, the print the new one? The user can edit the frame if they want to amend the note. It will be obvious and they will have a copy of the old note, if needed.
My hypothesis is overwriting will be more common. When you start a task you might have a general idea of what to do but not the full picture. By the end of the task you can describe exactly what happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for a warning + dropping it 👍 . I guess people will either pass a message to start or stop, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small comments but otherwise it looks great 👍 . Thanks for the hard work, and sorry for the delay!
Thanks for the review! I’m swamped from work but will try to find time to address the feedback in the coming week. |
Any news on this? |
I'm excited about using the functionality in this PR, thanks for all your hard work on it @prat0088! |
@k4nar If this can be merged, I am happy to follow up with a PR addressing your three latest comments:
|
I'll let @prat0088 decide if he can make it or prefer that you finish this awesome feature @joelostblom 🙏 That would make 4 external contributors working on this. You are awesome ✨ ❤️ 💪 |
Sorry I haven’t been active. Work has changed the way we log time and I no longer have a need for this program, but it is still by far my favorite way to log. If this can be merged as is I’ll merge it. If it’s decided changes are needed first then I don’t mind if someone branches this branch, adds the changes, and commits. |
Thanks for the quick feedback @prat0088! I would prefer to merge this feature once fully achieved, so @joelostblom feel free to add your fixes to a new branch and open a new PR 🙏 |
@jmaupetit Done! See #383 |
Closing this in favor to #383 |
Copied from @jnsebgosselin 's work on #252 and rebased.
To those coming from the feature request issues, you can install this now to play around with by running by running
It has been working fine for me on Windows. But please be warned this change hasn't yet been merged into master so there's no guarantees it will continue to work or that it won't corrupt your existing data.
Closes #93