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

Add support for loading a datetime value #55

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

Conversation

amaraliou
Copy link

Currently, there is no way to pass a datetime environment variable. The closest thing we have is the :date type which one can only use for dates. We have recently hit a use case where it would be ideal to support loading :datetime environment variables (we want to set a date and time after which some business logic changes).

This PR adds just that + the validation required to load a datetime value correctly.

Currently, there is no way to pass a datetime environment variable. The closest thing we have is the `:date` type which one can only use for dates. We have recently hit a use case where it would be ideal to support loading `:datetime` environment variables (we want to set a date and time after which some business logic changes).

This commit adds just that + the validation required to load a datetime value correctly.
value = load_string(name, required)
return nil if value.nil?

DateTime.parse(value)
Copy link
Contributor

@ameykusurkar ameykusurkar Jun 27, 2024

Choose a reason for hiding this comment

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

Looking at the documentation:

This method does not function as a validator. If the input string does not match valid formats strictly, you may get a cryptic result. Should consider to use DateTime.strptime instead of this method as possible.

Should we use a standardised format (eg. DateTime.rfc3339(...)) to avoid ambiguity?

Copy link
Contributor

Choose a reason for hiding this comment

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

Optionally, we can call the arguument :datetime_rfc3339 if we want.

Copy link

@benk-gc benk-gc Jun 27, 2024

Choose a reason for hiding this comment

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

Petition to use ActiveSupport::TimeWithZone. It has proper timezone support, and we can safely (de)serialize values with the iso8601 method which is our general standard elsewhere (e.g. in Pub/Sub messages).

Copy link

@kevinrobayna kevinrobayna Jun 27, 2024

Choose a reason for hiding this comment

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

I came here to talk about the above.

Date.parse or DateTime.parse are Russian roulettes so we def need to use a specific format and agree on it.

Choose a reason for hiding this comment

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

ActiveSupport::TimeWithZone

that would mean adding rails as a dependency, not sure we should do that.

Copy link

@benk-gc benk-gc Jun 27, 2024

Choose a reason for hiding this comment

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

@kevinrobayna I think we can just add active_support as a dependency and require the minimal active_support requirements and active_support/time_with_zone like so:

require "active_support"
require "active_support/time_with_zone"

See https://guides.rubyonrails.org/active_support_core_extensions.html#stand-alone-active-support

Choose a reason for hiding this comment

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

that is "rails" 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

So I just found this:

DateTime class is considered deprecated. Use Time class.

I'm in two minds about adding a big dependency to a very minimal gem as well. Granted that TimeWithZone comes with a bunch of goodies, but looks like Time does handle the basics of timezones correctly:

irb(main):001> require "time"
=> true
irb(main):002> t = Time.iso8601("2023-10-07T21:15:30+11:00")
=> 2023-10-07 21:15:30 +1100
irb(main):003> t.utc
=> 2023-10-07 10:15:30 UTC

Plus if this gem is used in the context of a Rails app, then we can just do t.in_time_zone to get a full-fledged ActiveSupport::TimeWithZone, so probably not too bad?

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.

4 participants