Allow floating point, negative, and second unit times for start param… #140
Conversation
|
What is the use case of querying for events in the future? |
|
Huh... I was just trying to support fractional values, and threw in the +/-
at the front as potentially valid number strings.
I'm not sure there is any point to dates in the future, unless your system
clock is messed up :).
…On Sat, Jul 29, 2017 at 9:20 AM, Jorge Bastida ***@***.***> wrote:
What is the use case of querying for events in the future?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#140 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEQb0e_NZ1OcdIYfEgku2B_QelgdGclIks5sS1u5gaJpZM4OEMzu>
.
|
| return None | ||
|
|
||
| ago_regexp = r'(\d+)\s?(m|minute|minutes|h|hour|hours|d|day|days|w|weeks|weeks)(?: ago)?' | ||
| ago_regexp = r'([+-]?([0-9]*[.])?[0-9]+)\s?(s|sec|secs|second|seconds|m|minute|minutes|h|hour|hours|d|day|days|w|weeks|weeks)(?: ago)?' |
There was a problem hiding this comment.
I think I agree that the +/- is redundant.
The digit pattern may be a bit more efficient (not a practical concern) as ([0-9]+(?:\.[0-9]+)?) - it would also avoid the additional matching group, so you just have (amount, unit) again.
To be consistent the additional time spans would be s|second|seconds, as there's nothing like hr|hrs.
The line is getting long (enough to cause the linter to kick off in Travis), a change to s|seconds?|m|minutes?|… seems reasonable.
p.s. it looks like there was a bug previously, was the pattern ends with weeks|weeks instead of week|weeks. It would also be good to add $ to the end of the pattern.
There was a problem hiding this comment.
- I agree the +/- is a bit too much.
- I do agree with @Code0x58 about the regexp and
s|second|seconds
| amount = int(amount) | ||
| unit = {'m': 60, 'h': 3600, 'd': 86400, 'w': 604800}[unit[0]] | ||
| amount, whole_part, unit = ago_match.groups() | ||
| try: |
There was a problem hiding this comment.
This try/except block could be replaced with just amount = float(amount) as the precision is good enough for this application regardless.
There was a problem hiding this comment.
It is a long while since the PR was sent (sorry). @Code0x58 @digitalmacgyver would any of you mind updating and sending this PR again ?
| return None | ||
|
|
||
| ago_regexp = r'(\d+)\s?(m|minute|minutes|h|hour|hours|d|day|days|w|weeks|weeks)(?: ago)?' | ||
| ago_regexp = r'([+-]?([0-9]*[.])?[0-9]+)\s?(s|sec|secs|second|seconds|m|minute|minutes|h|hour|hours|d|day|days|w|weeks|weeks)(?: ago)?' |
There was a problem hiding this comment.
- I agree the +/- is a bit too much.
- I do agree with @Code0x58 about the regexp and
s|second|seconds
| amount = int(amount) | ||
| unit = {'m': 60, 'h': 3600, 'd': 86400, 'w': 604800}[unit[0]] | ||
| amount, whole_part, unit = ago_match.groups() | ||
| try: |
|
I'm long gone, so can't offer any more time on this project. This could be a nice improvement |
4 participants
This change allows the the --start parameter to take on:
values starting with + or -
floating point values with . in them
values denominated in seconds
like:
'-1 min ago' (1 minute in the future)
'2.5 weeks ago'
'+.125 s' (1/8th of a second ago)
It does not allow numbers with a terminal . like: 123.