Fixing discrepancy between date.to_s(:long) and localize(date, format: :long) #14245

Open
wants to merge 11 commits into
from

Conversation

Projects
None yet
7 participants
Contributor

oliveiraethales commented Mar 2, 2014

This is an attempt to fix #14237 using the format string kindly provided by @gabrielecirulli

Feedback and review is welcome <3

@pixeltrix pixeltrix and 1 other commented on an outdated diff Mar 2, 2014

activesupport/test/core_ext/date_ext_test.rb
@@ -28,6 +28,12 @@ def test_to_s
assert_equal "2005-02-21", date.to_s(:iso8601)
end
+ def test_to_s_one_digit_day
+ date = Date.new(2005, 2, 2)
+
@pixeltrix

pixeltrix Mar 2, 2014

Owner

I don't think we need this empty line

@oliveiraethales

oliveiraethales Mar 2, 2014

Contributor

Oops, that one got unnoticed. Thanks for pointing out, will fix it (:

@pixeltrix pixeltrix and 1 other commented on an outdated diff Mar 2, 2014

activesupport/test/i18n_test.rb
@@ -26,7 +26,7 @@ def test_date_localization_with_short_format
end
def test_date_localization_with_long_format
- assert_equal @date.strftime("%B %d, %Y"), I18n.localize(@date, :format => :long)
+ assert_equal @date.strftime("%B %-e, %Y"), I18n.localize(@date, :format => :long)
@pixeltrix

pixeltrix Mar 2, 2014

Owner

I prefer to use literal expected values - using an dynamic one makes it possible that a bug in strftime goes unnoticed. Can you convert all the @date and @time values in this test file to literal ones - thanks!

@oliveiraethales

oliveiraethales Mar 2, 2014

Contributor

Sure! Will be a pleasure :D

👍 on this!

Contributor

oliveiraethales commented Mar 2, 2014

@pixeltrix Done! Let me know if there's anything else before squashing (:
Btw, does this requires a Changelog update?

@pixeltrix pixeltrix and 1 other commented on an outdated diff Mar 2, 2014

activesupport/test/i18n_test.rb
@@ -10,39 +10,39 @@ def setup
def test_time_zone_localization_with_default_format
now = Time.local(2000)
- assert_equal now.strftime("%a, %d %b %Y %H:%M:%S %z"), I18n.localize(now)
+ assert_equal "Sat, 01 Jan 2000 00:00:00 -0200", I18n.localize(now)
@pixeltrix

pixeltrix Mar 2, 2014

Owner

This test fails on Travis because the default timezone there is UTC - you need to make the test independent of the timezone. Either set the timezone and restore it with an ensure block or use Time#in_time_zone to convert it to a known one - the latter is easiest in this case.

@oliveiraethales

oliveiraethales Mar 3, 2014

Contributor

Nice, thanks for pointing that one! :D
Just to check if I understood it correctly... using something like 'I18n.localize(now.in_time_zone('Alaska')' would be a valid way?

@pixeltrix

pixeltrix Mar 3, 2014

Owner

using something like 'I18n.localize(now.in_time_zone('Alaska')'

Use it with Time.utc when creating the local variable, e.g:

now = Time.utc(2000).in_time_zone('Alaska')

otherwise you'll still be system configuration dependent.

@oliveiraethales

oliveiraethales Mar 3, 2014

Contributor

Nice! Thanks for the insight :3 Will do that

Owner

pixeltrix commented Mar 2, 2014

Btw, does this requires a Changelog update?

Yes, since we're changing the default to not including zeros.

Do you guys think it might also make sense to change the :long format for Time to make use of %-e instead of %d (as it currently does) on the grounds of consistency?

en:
  time:
    formats:
      long: "%B %d, %Y %H:%M"
Owner

pixeltrix commented Mar 2, 2014

@gabrielecirulli yes, they should be consistent - good catch.

Contributor

oliveiraethales commented Mar 3, 2014

There you go guys! Please let me know if there's anything else you guys think might be good. (:

Hey @oliveiraethales! Great work! I hope this is not too much to ask, I think we should also consider fixing the discrepancies between the other formats, as they seem to be even bigger. I've highlighted them for you and provided solutions that you could use:


:short format

date = Date.new(2000, 2, 1)

date.to_s(:short)            #=> " 1 Feb"
I18n.l(date, format: :short) #=> "Feb 01"

The format used by Date#to_formatted_s (%e %b) causes the number to be padded with empty spaces. I believe it's undesirable behavior.

The format uses by I18n::localize is saner but the number is still padded with zeroes. In my opinion, that doesn't look very natural.

In this case, I would suggest that the %b %-e format is used instead:

date.strftime("%b %-e") #=> "Feb 1"

In the same way, the :short time format in en.yml should become:

en:
  time:
    formats:
      short: "%b %-e %H:%M"

:default format

This one is a bit tougher. Date#to_formatted_s uses Ruby's default Date#to_s behavior when the format is :default, and I think we shouldn't modify that. The output in that case is:

date = Date.new(2000, 2, 1)

date.to_s(:default)                    #=> "2000-02-01"
I18n.l(date, format: :default)         #=> "2000-02-01"

date.to_time.to_s                      #=> "2000-02-01 00:00:00 +0100"
I18n.l(date.to_time, format: :default) #=> "Tue, 01 Feb 2000 00:00:00 +0100"

I believe the only thing it makes sense to change is the :default time format in en.yml, which is completely inconsistent with the rest. Here's what I think it should look like:

en:
  time:
    formats:
      default: "%Y-%m-%d %H:%M:%S %z"

The output of that format is: "2000-02-01 00:00:00 +0100"


I think that's it as far as modifications go 😄

The other formats in Date::DATE_FORMATS don't really have a counterpart in I18n::localize formats, so I think there's not much to do.

I think this change might also cause some issues for people who rely on the default behavior and update their Rails app to the latest version, so I think it might make sense to note the change on upgrading guides if possible.

Contributor

oliveiraethales commented Mar 3, 2014

Hey mate! :D What a lovable research, very nice man! :3
I agree with upgrading the guides, I'll happily do them also!

I just don't know if it would be good to continue using this PR for this or if another one should be opened. What do you think @pixeltrix ?

Owner

pixeltrix commented Mar 3, 2014

Keep it in the same request - easier to revert if necessary

Contributor

oliveiraethales commented Mar 3, 2014

Alright! Will update the other formats and the docs also.

@oliveiraethales Please note that I made a small mistake in my suggestion for en.yml in the case of the :default time format: I forgot to include hours, minutes and seconds in the format.

I've updated my previous comment with the correct format and I'll also paste it here for clarity:

en:
  time:
    formats:
      default: "%Y-%m-%d %H:%M:%S %z"

Sorry for the mess!

Contributor

oliveiraethales commented Mar 4, 2014

Hey guys! There it is, fixed the other formats and updated the guides also.
Please review and let me know it there's anything else :3

@pixeltrix pixeltrix commented on the diff Mar 4, 2014

actionview/test/template/date_helper_test.rb
@@ -3197,7 +3197,7 @@ def test_time_tag_with_given_block
def test_time_tag_with_different_format
time = Time.new(2013, 2, 20, 0, 0, 0, '+00:00')
- expected = '<time datetime="2013-02-20T00:00:00+00:00">20 Feb 00:00</time>'
+ expected = '<time datetime="2013-02-20T00:00:00+00:00">Feb 20 00:00</time>'
@pixeltrix

pixeltrix Mar 4, 2014

Owner

I'm not sure about changing the order of day and month in the short format - I think that may be one standardisation too far

Owner

pixeltrix commented Mar 4, 2014

Other than the question of the order of the short format it looks good - obviously the commits need to be squashed. I'll ask the other team members about the short format and get back to you.

Well, the rationale behind this could be that its's better to have everything standardised to avoid any gotchas, but if any developer wants the old behaviour back we could still offer a list with the old localisations in the guides maybe.

robin850 added this to the 4.2.0 milestone Mar 8, 2014

Contributor

oliveiraethales commented Mar 9, 2014

Hey @pixeltrix! (:

Sorry to bother, but have you been able to confirm about the short format?

Thanks a lot!

Member

robin850 commented Mar 9, 2014

I did not comment yet because I was not sure that everything was finished but could you please just wrap your guide and changelog additions around 80 chars please ? Beside this, it looks great, thank you for the work here! 👍

Contributor

oliveiraethales commented Mar 9, 2014

Sure thing @robin850! :D Will do that

Contributor

pftg commented Mar 9, 2014

@oliveiraethales please squash commits too

Contributor

oliveiraethales commented Mar 9, 2014

Yup, I'm just waiting for @pixeltrix considerations and then I'll squash (:

@rafaelfranca rafaelfranca modified the milestone: 4.2.0, 5.0.0 Aug 18, 2014

sgrif was assigned by rails-bot Oct 20, 2015

Member

sgrif commented Dec 15, 2015

@rails-bot rails-bot assigned pixeltrix and unassigned sgrif Dec 15, 2015

rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016

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