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

offset_for_local_datetime: Add a 'ignore_missing_spans' argument #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wolfsage
Copy link
Contributor

If this is true and the local time for the given $dt object does
not exist in the time zone (due to DST changes for example), the previous
span will be returned.

# return the previous span so the offset to utc is higher,
# effectively moving the time forward whatever the difference
# in the two spans is (typically 1 hour for DST).
return $self->{spans}[ $i - 1 ] if $ignore_missing_spans;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is $i - 1 good enough (since $i can be > $max, not just == $max) ...

If this is true and the local time for the given $dt object does
not exist in the time zone (due to DST changes for example), the previous
span will be returned.
@wolfsage wolfsage force-pushed the ignore-missing-span branch from aac937e to 1f28fa7 Compare May 9, 2018 13:54
Copy link
Member

@autarch autarch left a comment

Choose a reason for hiding this comment

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

This failed the Travis tests because the code is not passing the tidyall check. See the contributing doc for details on how to fix this.


Given a C<DateTime> object, this method returns the offset in seconds
for the given datetime. Unlike the previous method, this method uses
the local time's Rata Die days and seconds. This should only be done
when the corresponding UTC time is not yet known, because local times
can be ambiguous due to Daylight Saving Time rules.

If C<$ignore_missing_spans> is true and the local time for C<$dt> does not
Copy link
Member

@autarch autarch Jun 8, 2018

Choose a reason for hiding this comment

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

I wonder if it makes sense to document this talking about "spans". Note that the docs do not mention this term at all, because it's really just an internal detail.

I think this might be better presented as $fall_forward_on_skipped_time or something like that. Saying "the next span up" seems wrong too. Aren't we going to the previous span? But anyway, I'd say something like this:

If $fall_back_on_skipped_time is true and the local time for C<$dt> does not exist in the time zone (typically because of a DSST change), then we will fall back to nearest previous offset for the local time.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we're going to the previous span but that moves the time forward, so I'd add to the above ...

Note that moving to a previous offset will typically move the time forward, since the new offset from UTC will be larger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants