Skip to content

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.

2 participants