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

Fix repr on DateTime #30200

Merged
merged 13 commits into from
Dec 18, 2018
Merged

Fix repr on DateTime #30200

merged 13 commits into from
Dec 18, 2018

Conversation

sam0410
Copy link
Contributor

@sam0410 sam0410 commented Nov 29, 2018

An Attempt to fix #29909
Thanks.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Looks good to me. Could probably use another pair of eyes that's more familiar with the dates code.

@@ -400,29 +400,29 @@ As a bonus, all period arithmetic objects work directly with ranges:

```jldoctest
julia> dr = Date(2014,1,29):Day(1):Date(2014,2,3)
2014-01-29:1 day:2014-02-03
Date(2014, 1, 29):1 day:Date(2014, 2, 3)
Copy link
Member

Choose a reason for hiding this comment

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

These changes seems like an unintended consequence? At least to me it is a step back in readability.

Copy link
Member

Choose a reason for hiding this comment

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

No, the idea is to show dates and times in a format that is valid input syntax. The fact that the step doesn't quite use valid input syntax is the remaining issue here—if that was shown as Day(1) instead then this whole output would be valid input syntax. Printing date ranges could use a more human-readable format instead of a valid input syntax.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, placed the comment on the wrong line; do we want the new printing from the collect on the next lines?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, I guess we can use either format there. This seems fine to me though.

stdlib/Dates/src/io.jl Outdated Show resolved Hide resolved
stdlib/Dates/src/io.jl Outdated Show resolved Hide resolved
@omus omus added the dates Dates, times, and the Dates stdlib module label Dec 4, 2018
@omus
Copy link
Member

omus commented Dec 4, 2018

I noticed with this PR Dates in DataFrames display the valid input syntax and not the human readable form:

julia> DataFrame(:date => today(), :value => rand())
1×2 DataFrame
│ Row │ date              │ value    │
│     │ Date              │ Float64  │
├─────┼───────────────────┼──────────┤
│ 1Date(2018, 12, 4) │ 0.615755

If we make the :compact version of show call print then would get the terse 2018-12-04 version in the DataFrame.

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

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

Changes look good to me. I'd like someone to chime in on if the :compact behavioural differences make sense (I think they do). The current behaviour of this PR:

julia> using Dates, DataFrames

julia> d = today()
2018-12-07

julia> print(d)
2018-12-07
julia> show(d)
Date(2018, 12, 7)

julia> fill(d, 2)
2-element Array{Date,1}:
 Date(2018, 12, 7)
 Date(2018, 12, 7)

julia> fill(d, 2, 2)
2×2 Array{Date,2}:
 2018-12-07  2018-12-07
 2018-12-07  2018-12-07

julia> DataFrame(:date => today(), :value => rand())
1×2 DataFrame
│ Row │ date       │ value    │
│     │ Date       │ Float64  │
├─────┼────────────┼──────────┤
│ 12018-12-070.657502

@sam0410
Copy link
Contributor Author

sam0410 commented Dec 7, 2018

Thanks @omus

@JeffBezanson JeffBezanson added the minor change Marginal behavior change acceptable for a minor release label Dec 14, 2018
@JeffBezanson JeffBezanson added this to the 1.2 milestone Dec 14, 2018
@sam0410
Copy link
Contributor Author

sam0410 commented Dec 17, 2018

Hi everyone, Is this ready for merge now?

@ararslan ararslan added the needs news A NEWS entry is required for this change label Dec 17, 2018
@ararslan
Copy link
Member

The changes look okay to me, but this will need an entry in NEWS.md.

sam0410 and others added 2 commits December 18, 2018 03:03
@sam0410
Copy link
Contributor Author

sam0410 commented Dec 17, 2018

Thanks for your review @ararslan. I added the news entry.

@ararslan ararslan removed the needs news A NEWS entry is required for this change label Dec 17, 2018
@omus omus merged commit 8d8b3d9 into JuliaLang:master Dec 18, 2018
@omus
Copy link
Member

omus commented Dec 18, 2018

@sam0410 thanks for the contribution! 👍

@sam0410 sam0410 deleted the datePR branch December 18, 2018 19:02
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Mar 29, 2019
Related to these Dates stdlib changes:

- JuliaLang/julia#30200
- JuliaLang/julia#30817
omus added a commit to JuliaTime/TimeZones.jl that referenced this pull request Mar 29, 2019
* Overhaul printing of types

Related to these Dates stdlib changes:

- JuliaLang/julia#30200
- JuliaLang/julia#30817

* Review comments
KristofferC added a commit that referenced this pull request May 10, 2019
@omus omus mentioned this pull request May 21, 2019
KristofferC pushed a commit that referenced this pull request Sep 6, 2019
This reverts commit 8d8b3d9.
KristofferC added a commit that referenced this pull request Sep 6, 2019
JeffBezanson added a commit that referenced this pull request Dec 16, 2019
This reverts commit 8d8b3d9.

Conflicts:
	NEWS.md
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
This reverts commit 8d8b3d9.

Conflicts:
	NEWS.md
kpamnany pushed a commit to RelationalAI-oss/TimeZones.jl that referenced this pull request May 5, 2023
* Overhaul printing of types

Related to these Dates stdlib changes:

- JuliaLang/julia#30200
- JuliaLang/julia#30817

* Review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using repr on DateTime shows pretty-printed version
6 participants