Skip to content

Simplify #inspect - #12

Merged
jeremyevans merged 2 commits into
ruby:masterfrom
zverok:simplify-inspect
Oct 18, 2019
Merged

Simplify #inspect#12
jeremyevans merged 2 commits into
ruby:masterfrom
zverok:simplify-inspect

Conversation

@zverok

@zverok zverok commented Aug 11, 2019

Copy link
Copy Markdown
Contributor

I'd like to raise a discussion on the following change:

# before:
Date.new(2010, 1, 3).inspect
# => #<Date: 2010-01-03 ((2455200j,0s,0n),+0s,2299161j)> 

# after:
Date.new(2010, 1, 3).inspect
# => #<Date: 2010-01-03> 

Justification: for most of contexts where Date could be used (web apps, data conversion scripts, experiments and so on), the "((2455200j,0s,0n),+0s,2299161j)" suffix is meaningless, and just obscures the data reading. Imagine the (pretty simple) example:

# before
(1..10).map { |days| Date.today - days }
# => [#<Date: 2019-08-10 ((2458706j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-09 ((2458705j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-08 ((2458704j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-07 ((2458703j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-06 ((2458702j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-05 ((2458701j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-04 ((2458700j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-03 ((2458699j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-02 ((2458698j,0s,0n),+0s,2299161j)>, #<Date: 2019-08-01 ((2458697j,0s,0n),+0s,2299161j)>]

# after:
(1..10).map { |days| Date.today - days }
 => [#<Date: 2019-08-10>, #<Date: 2019-08-09>, #<Date: 2019-08-08>, #<Date: 2019-08-07>, #<Date: 2019-08-06>, #<Date: 2019-08-05>, #<Date: 2019-08-04>, #<Date: 2019-08-03>, #<Date: 2019-08-02>, #<Date: 2019-08-01>]

Whether it is a list of dates we extract from DB, or have in test (of some scheduling library), or parse from CSV, I believe the "before" example is very hard to read and provides no additional value.

On the other hand, for the rare contexts when values like "julian day number" matter, the ((2458697j,0s,0n),+0s,2299161j) format seem to be too terse to be easily used by anybody not 200% familiar with library's internals.

@jeremyevans

Copy link
Copy Markdown
Contributor

I agree this makes more sense and will make the inspect output easier to read. I plan to merge this before 2.7 is released unless there are objections.

@jeremyevans
jeremyevans merged commit 8c02586 into ruby:master Oct 18, 2019
@zverok
zverok deleted the simplify-inspect branch October 18, 2019 20:18
@nurse

nurse commented Oct 25, 2019

Copy link
Copy Markdown
Member

I object this change.
inspect should suggest its internal and it should keep julian day number.

If you want to introduce such Web friendly date library, don't build it on top of current date library.
Please create new one on top of Time.

@jeremyevans

Copy link
Copy Markdown
Contributor

@nurse I can revert this, however, first please consider that "inspect should suggest its internal" is no longer a reason to do so. The date library since 1.9.3 does not store the julian day number for dates in the typical case where it is initialized via Date.new/Date.civil or Date.today. If, after considering that, you would still like this reverted, please let me know and I will revert this.

@nurse

nurse commented Oct 26, 2019

Copy link
Copy Markdown
Member

@jeremyevans The fact it doesn't calculate julian day number is implementation detail.
Of course you have a question, what is the difference between "internal" and "implementation detail".

I think Date#inspect should show the difference between Date.today(Date::ITALY) and Date.today(Date::ENGLAND).

# master
irb(main):002:0> Date.today(Date::ITALY)
=> #<Date: 2019-10-27>
irb(main):003:0> Date.today(Date::ENGLAND)
=> #<Date: 2019-10-27>
# 2.6
irb(main):002:0> Date.today(Date::ITALY)
=> #<Date: 2019-10-27 ((2458784j,0s,0n),+0s,2299161j)>
irb(main):003:0> Date.today(Date::ENGLAND)
=> #<Date: 2019-10-27 ((2458784j,0s,0n),+0s,2361222j)>

@jeremyevans

Copy link
Copy Markdown
Contributor

@nurse OK. I'll revert this change.

@jeremyevans

Copy link
Copy Markdown
Contributor

Revert committed at 875d563.

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.

3 participants