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

Set correct day when year or month is changed too #671

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

oliver-kriska
Copy link

Summary of changes

  • when you call Timex.set/2 function from date which has less days than new month it should apply month or year changes before day changes so validation will be correct. So options have to be in exact order: year, month, day
  • added alias for Helpers module

Example before this fix:

Timex.now() |> Timex.set(day: 31, month: 5)
~U[2021-05-30 17:49:07.917681Z]
Timex.now() |> Timex.set(month: 5, day: 31)
~U[2021-05-31 17:49:10.264841Z]

There you can see correct date is only when I change order of options.

Checklist

  • New functions have typespecs, changed functions were updated
  • Same for documentation, including moduledocs
  • Tests were added or updated to cover changes
  • Commits were squashed into a single coherent commit
  • Notes added to CHANGELOG file which describe changes at a high-level

Oliver Kriska added 3 commits June 2, 2021 21:58
 - when you call `Timex.set/2` function from date which has less days than new month it should apply month or year changes before day changes so validation will be correct. So options have to be in exact order: year, month, day
 - added alias for Helpers module
 - when you call `Timex.set/2` function from date which has less days than new month it should apply month or year changes before day changes so validation will be correct. So options have to be in exact order: year, month, day
 - added alias for Helpers module
@oliver-kriska
Copy link
Author

I did some cosmetic changes so there 3 commits. In case it will be ok, I can make new PR with one commit.

Copy link
Owner

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this behavior is documented, I think this is a worthwhile change to implement. Two things:

  • I left one suggested change I'd like to have you make
  • Could you also rebase/squash your commits? You don't have to break up the cosmetic changes at this point (please do in the future though)

Thanks for the PR!

lib/datetime/helpers.ex Show resolved Hide resolved
@oliver-kriska
Copy link
Author

I added your suggestion code, all tests passed. It looks now better. Thanks for good suggestion.

Sorry but I don't understand what you want with this:

Could you also rebase/squash your commits? You don't have to break up the cosmetic changes at this point (please do in the future though)

I updated my main branch and now it's code base same as in your master, plus my changes. I don't like doing hard reset and creating one commit. Because it can be done when this PR will be merged. Because we would lost history about the changes and I'm not sure about this PR, maybe it will just update PR or I would have to create new one after reset and squash commits. Maybe I'm wrong so sorry and please give me a advice.
CleanShot 2021-08-03 at 10 19 22

@Zarathustra2
Copy link

Any chance this can be merged?

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

4 participants