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

[bugfix] Fixes #3717, ensure day-of-year is non-zero #3787

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@marwahaha
Member

marwahaha commented Feb 26, 2017

@icambron

This comment has been minimized.

Member

icambron commented Feb 26, 2017

This makes sense, though the regex is a little unfortunate. My only question is whether it would have been cleaner to allow zero to pass through the parsing and flag it further downstream. For example, we do allow numbers like 455 through at parsing and just mark it as invalid in a later step (overflow, I guess?). Maybe that's a cleaner spot for this check to. Put another way, why is it 1-999 instead of 1-365?

@marwahaha

This comment has been minimized.

Member

marwahaha commented Feb 26, 2017

For sure @icambron . A lot simpler that way

@icambron

This comment has been minimized.

Member

icambron commented Feb 26, 2017

Yeah, I like that more. Thanks! This looks good to me, @ichernev & @maggiepint (or whoever does merges/marks for release now).

@maggiepint

This comment has been minimized.

Member

maggiepint commented Feb 28, 2017

Sure. Makes sense.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Mar 2, 2017

Merged in c338954

@ichernev ichernev changed the title from Force day-of-year to nonzero parsing to [bugfix] Fixes #3717, ensure day-of-year is non-zero Mar 2, 2017

@ichernev ichernev closed this Mar 2, 2017

ichernev added a commit that referenced this pull request Mar 2, 2017

Merge pull request #3787 from marwahaha:fix/3717
[bugfix] Fixes #3717, ensure day-of-year is non-zero
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment