time only support for watson add #282
Conversation
|
Looks like we finally have our first CLI tests 😉 |
|
Do you know why Github is displaying such a long diff for |
watson/cli.py
Outdated
|
|
||
| def getMergedDateTime(date_time, time_string): | ||
| hours, minutes = time_string.split(":") | ||
| return date_time.shift(hours=int(hours), minutes=int(minutes)) |
There was a problem hiding this comment.
Those functions should probably go to the utils module (and be unit tested ;) ).
| to = getMergedDateTime(from_.floor("day"), to) | ||
| elif isTime(from_) and isTime(to): | ||
| from_ = getDateTimeToday(from_) | ||
| to = getDateTimeToday(to) |
There was a problem hiding this comment.
I wonder if Arrow doesn't provide something to help with that. It feels a bit tedious to introduce our own regexp to parse the date & times.
There was a problem hiding this comment.
I have checked the api and didn't find s.th. appropriate. But I am open for suggestions.
There was a problem hiding this comment.
I've been looking into it and there's something I don't understand: why are there two different click types Date and Time in cli.py?
It turns out that both accept the full date, but using different formats!
TimeParamType(aka,Time) is only used inwatson stop:
$ watson stop --at 09:00
$ watson stop --at 2019-05-23T12:00
DateParamType(aka,Date) is used in every other place where a date is required:
$ watson report -f 2019-05-23
$ watson report -f "2019-05-23 12:00"
$ watson report -f "2019-05-23T12:00"
In my opinion, the following changes might solve issue #278 in a simpler way:
- Rename
DatetoDateTimewhich is more appropriate to what it does. Of course, rename also the correspondingParamTypeclasses. - Allow
DateTimeto receive time only dates, which is precisely what theTimetype does. You could reuse the code. - Use
DateTimewhereTimewas used (i.e.watson stop). - Remove
TimeandTimeParamType.
Please, remember that I'm not a member of this project, so I might be missing important stuff!! :-) what do you think @k4nar?
I think date/time input format is one of the aspects where Watson could get a nice improvement, so it's great that there's someone working on it!
There was a problem hiding this comment.
I like the idea. I didn't dive deep so I was not aware of TimeParamType which is basically doing what I implemented to handle time only.
I also couldn't agree more that the api is not consistent for the different cmds and it felt like adding another balcony.
The only drawback I see so far is that it will be hard to handle the case where we want to reuse the date specified for --from for --to as the parameter processing would be done without the knowledge about the other parameter.
Yes I am on Windows. I use checkout-as-is and commit-as-is which suits most cases very well. Have pushed a new commit fixing the line endings. |
Improve usability of watson add (issue #278)