Skip to content
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

every 10 days of the year starting on the 65th day of the year #130

Closed
gautaz opened this issue Nov 4, 2015 · 8 comments
Closed

every 10 days of the year starting on the 65th day of the year #130

gautaz opened this issue Nov 4, 2015 · 8 comments

Comments

@gautaz
Copy link
Contributor

gautaz commented Nov 4, 2015

Hello,

Looking at the laterjs text parser, I was wondering why the rank regex was limited to 59.
In fact, it makes sense for all time units but for days in the year.
Is there any particular reason for this ?

@bunkat
Copy link
Owner

bunkat commented Nov 4, 2015

Nope, that is definitely an oversight. You're right that it should go up to 366 to cover all cases.

@gautaz
Copy link
Contributor Author

gautaz commented Nov 4, 2015

OK, actually there is a way to work around the issue with:
every 10 days of the year starting on the 0300 day of the year

@gautaz
Copy link
Contributor Author

gautaz commented Nov 4, 2015

Or to get back to the original example:
every 10 days of the year starting on the 0065 day of the year
But it seems a bit awkward...

@gautaz
Copy link
Contributor Author

gautaz commented Nov 4, 2015

Fixing this with a regex that checks english grammar seems overwhelming, perhaps:

/^((\d+)(st|nd|rd|th)?)\b/

would be enough for checking a rank?
If the user is not using the right cardinality suffix, I guess laterjs will still work as expected?

@bunkat
Copy link
Owner

bunkat commented Nov 5, 2015

Yes, this should work just fine. Laterjs just strips the suffix anyways so it isn't important that it is correct and I wasn't doing all of the checks that would have been required to make sure that the value provided was actually valid anyways.

@gautaz
Copy link
Contributor Author

gautaz commented Nov 6, 2015

Sorry for the delay,
@bunkat Do you want me to do a PR for this?

@bunkat
Copy link
Owner

bunkat commented Nov 13, 2015

If you want to, that would be great. Thanks!

@gautaz
Copy link
Contributor Author

gautaz commented Nov 15, 2015

Done :-). There is only one additional test case (two in fact for the day of year restriction), perhaps a few more would help...

@bunkat bunkat closed this as completed in 5f026a5 Nov 21, 2015
bunkat pushed a commit that referenced this issue Nov 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants