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

Fix current day and future month in SlashDMY #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mvgrimes
Copy link

When handling DD/MM without a year, SlashDMY returns nil for the current date
(ie, 15/7 in the tests) or for any future months (ie, 14/8).

This pull request adds failing tests and fixes fixes SlashDMY.

Thanks for publishing this package. I've found it quite useful.

If no year is specified, then nil is returned for:

- current date July 15 in test
- future months
@mvgrimes mvgrimes changed the title Fix current day and future month in ParseDMY Fix current day and future month in SlashDMY Apr 17, 2023
@olebedev
Copy link
Owner

olebedev commented May 31, 2023

Hello @mvgrimes,

Thank you for your contribution. However, the change you propose contradicts the purpose of the slash_dmy rule, which stands for day, month, year. We already have the dd/mm parser logic in place, as a part of the slash_dmy rule, which is unfortunate, I believe we have to remove that ambiguity from that rule and make it stricter.

I appreciate the idea of implementing such a rule, but I suggest creating a separate rule that is not enabled by default. The reason for this is that the tuples like dd/mm or mm/dd are even more vague than dd/mm/yyyy or mm/dd/yyyy. It becomes challenging to determine if these represent a specific point in time or something else.

Could you please rework it into a dedicated rule that is not enabled by default? Say, slash_dm?

Best regards,
Oleg

@olebedev
Copy link
Owner

If you are willing to continue with that, please do a git rebase over the resent master branch so we can see the CI checks running against the change.

@mvgrimes
Copy link
Author

mvgrimes commented Jun 2, 2023

Hi @olebedev,

The dmy rule isn't really something that I'll ever use. I just noticed that it worked for d/m if they were in the past, but returned nil for the current date and future dates. If you're interested in merging my other patches (#35 and #19), then I can take a look at separating out the dm rule from the dmy rule when I have some time.

@olebedev
Copy link
Owner

Hi @mvgrimes, I would be more that happy to get your patches in! Please do.

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

Successfully merging this pull request may close these issues.

None yet

2 participants