Allow spec
8000
ification of start time via start_at
#364
Conversation
|
@jmaupetit Just added a couple of minor updates. This is ready for review when you get a chance. Really enjoying watson so far and looking forward to contributing more if there is interest! Thanks again for making this great tool! |
watson/watson.py
Outdated
| @@ -251,7 +251,12 @@ def add(self, project, from_date, to_date, tags): | |||
| frame = self.frames.add(pro 8000 ject, from_date, to_date, tags=tags) | |||
| return frame | |||
|
|
|||
| def start(self, project, tags=None, restart=False, gap=True): | |||
| def start(self, project, tags=None, restart=False, start_at=None, gap=True): | |||
| if start_at is not None and not gap: | |||
There was a problem hiding this comment.
I noticed now that I could use the MutuallyExclusiveOption in the click decorator instead of this if statement. The only drawback would be that I didn't see a way to specify a custom error message which might be helpful, but it would still probably be better to handle this the same as other mutually exclusive options in watson and use the decorator. Let me know what you prefer.
There was a problem hiding this comment.
IMO we should use the decorator when applicable.
There was a problem hiding this comment.
Thanks, I changed this to use the decorator in my latest commit. Note the difference that the previous solution only raised an error if --at was used together with --no-gap, while the click decorator raises and error when used together with either --gap or --no-gap (it still works with the default parameter value of gap=True, it is just not possible to use the flag directly).
There was a problem hiding this comment.
Also just a note that there is apparently a click addon for options groups now https://github.com/click-contrib/click-option-group, including mutual exclusiveness and grouping in the help message, might be something worth checking out in the future.
|
@k4nar @jmaupetit I restructured my previous changes to fix the failed tests in my latest commit. The only failing test now is related to flake8 and also fails for me on master. Could you review this PR and let me know if you think the changes are sound before I try to add a test specifically for |
|
I added some suggested tests now also, ready for full review. I am not quite sure the test in |
| with pytest.raises(ValueError): | ||
| time_str = '2999-31-12T23:59' | ||
| # Task can't end in the future | ||
| with pytest.raises(WatsonError): |
There was a problem hiding this comment.
This was previously raisin ValueError because there is no month number 31. Now it correctly raises a WatsonError if the task starts in the future.
|
@jmaupetit @k4nar Just a reminder to review this PR when you get a chance. FWIW I have been using it locally without issues. |
|
We'll merge this after #369 🎉 |
|
Also: please update the CHANGELOG to add this new feature, yay! |
|
Thanks @jmaupetit ! I added a note in the changelog and rebased on #369 so should be good to go after that PR is merged. |
|
I think you are good to go for a new rebase |
Also fix previous changelog typo
Just to avoid confusion down the road.
Co-Authored-By: Julien Maupetit <jmaupetit@users.noreply.github.com>
Otherwise lots of tests need to be rewritten not to fail as there is no previous frame in the test data.
|
@jmaupetit Ready! |
Added: - Log output order can now be controlled via the `--reverse/--no-reverse` flag and the `reverse_log` configuration option (#369) - Add `--at` flag to the `start` and `restart` commands (#364). - Add `--color` and `--no-color` flags to force output to be colored or not respectively (#350). Changed: - Require latest Arrow version 0.15.6 to support ISO week dates (#380) Fixed: - Make after-edit-check ensure that edited time stamps are not in the future (#381)
Added: - Log output order can now be controlled via the `--reverse/--no-reverse` flag and the `reverse_log` configuration option (#369) - Add `--at` flag to the `start` and `restart` commands (#364). - Add `--color` and `--no-color` flags to force output to be colored or not respectively (#350). Changed: - Require latest Arrow version 0.15.6 to support ISO week dates (#380) Fixed: - Make after-edit-check ensure that edited time stamps are not in the future (#381)
A suggestion for how to implement
start_atsimilar to the already existingstop_at. @jmaupetitNote that there are 11 tests failing for me on the master branch, which is unchanged with the additions suggested here.
Close #75 #224 #352