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

Improve Date/DateTime interactions #62

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

milmazz
Copy link
Contributor

@milmazz milmazz commented Aug 1, 2023

This PR includes some improvements when interacting with Date or DateTime.

  • Elixir v1.15 introduced {Date,DateTime,NaiveDateTime,Time}.{after?/2,before?/2}. This means that we could improve the readability of some Date/Time comparisons.
  • I also took the chance to transform some Timex defdelegate to their native equivalent.

Finally, most of these additions only apply if we're running on Elixir v1.15, so I added new entries for Elixir and OTP matrix for CI purposes.

  • Please sign Adobe's CLA if this is your first time contributing to an Adobe open source repo. Thanks!

milmazz and others added 3 commits August 1, 2023 13:00
This PR includes some improvements when we interact with `Date` or
`DateTime`.

* Elixir v1.15 introduced `{Date,DateTime,NaiveDateTime,Time}.{after?/2,before?/2}`. This means that we could improve the readability on some Date/Time
comparisons.
* I also took the chance to transform some `Timex` `defdelegate` to
  their native equivalent.

Finally, most of these additions only apply if we're running on
Elixir v1.15, that's why I decided to add new entries for Elixir and OTP
matrix for CI purposes.
Copy link
Contributor

@novaugust novaugust left a comment

Choose a reason for hiding this comment

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

mind nixing the Timex compare bits and just leaving the 1.15 changes?

after thinking on it for a bit, i'm down with getting rid of Timex.now and Timex.today in favor of their explicit versions 👍

Comment on lines +99 to +103
defp style({{:., dm, [{:__aliases__, am, [:Timex]}, :today]}, funm, args}),
do: {{:., dm, [{:__aliases__, am, [:Date]}, :utc_today]}, funm, args}

defp style({{:., dm, [{:__aliases__, am, [:Timex]}, :now]}, funm, args}),
do: {{:., dm, [{:__aliases__, am, [:DateTime]}, :utc_now]}, funm, args}
Copy link
Contributor

Choose a reason for hiding this comment

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

let me sit on these ones -- i'd considered doing the same earlier, but nixed it as too specific to adbe internals 🤔 still, this repo is all about forcing our views on others xD

lib/style/single_node.ex Outdated Show resolved Hide resolved
lib/style/single_node.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@novaugust novaugust left a comment

Choose a reason for hiding this comment

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

this is great, ty milton!

@novaugust novaugust merged commit eb4319c into adobe:main Aug 1, 2023
1 check passed
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