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

[DOC] New doc about Julian/Gregorian #70

Merged
merged 5 commits into from
Aug 5, 2022
Merged

[DOC] New doc about Julian/Gregorian #70

merged 5 commits into from
Aug 5, 2022

Conversation

BurdetteLamar
Copy link
Member

Adding a new free-standing doc about Julian and Gregorian calendars, also covering the argument start that's used by some methods in Date.

The goal is to push discussion of calendars and start out of the general discussion, where they are of little interest to many (most?) users. Instead, links would take the reader off-page, when relevant.

Also: corrects two errors in date_core.c. (If this PR is rejected, I'll put up these corrections separately.)

Copy link
Member

@peterzhu2118 peterzhu2118 left a comment

Choose a reason for hiding this comment

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

I'm in favor of this change but why not directly change the documentation in the Date class rather than creating a separate document for this change? Many parts of the calendars.rdoc file already exists in the documentation for the Date class so this duplicates the existing documentation.

@BurdetteLamar
Copy link
Member Author

I'm in favor of this change but why not directly change the documentation in the Date class rather than creating a separate document for this change? Many parts of the calendars.rdoc file already exists in the documentation for the Date class so this duplicates the existing documentation.

Sorry I was not more clear about what I'm trying to do.

The problem I see is that in reading the overview of the Date class, the reader comes to the large and complicated section "Argument start", which I think is irrelevant to most users. (How many applications deal with dates going back centuries?)

I think this material should not be in the overview, but should instead be in a separate doc that gets link to from each relevant method doc.

I intend to remove that section from the overview, and to change the links to point to the section in the new doc.

Does it make better sense to go ahead with those additional changes in this PR?

@peterzhu2118
Copy link
Member

I see, that makes sense. I think it would also make sense to remove that part in the class doc for Date in the this PR so that we don't end up with duplicate documentation.

@BurdetteLamar
Copy link
Member Author

I see, that makes sense. I think it would also make sense to remove that part in the class doc for Date in the this PR so that we don't end up with duplicate documentation.

Done.

Copy link
Member

@peterzhu2118 peterzhu2118 left a comment

Choose a reason for hiding this comment

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

Can you also add the doc directory into the .document file so that it's included when generating RDoc.

Comment on lines 67 to 77
# Julian date, incremented to a Gregorian date.
d0 = Date.new(1582, 1, 1, Date::ITALY) # => #<Date: 1582-01-01>
d0.julian? # => true
d1 = d0.next_year # => #<Date: 1583-01-01 >
d1.gregorian? # => true

# Gregorian date, decremented to a Julian date.
d0 = Date.new(1583, 1, 1, Date::ITALY) # => #<Date: 1583-01-01)>
d0.gregorian? # => true
d1 = d0.prev_year # => #<Date: 1582-01-01>
d1.julian? # => true
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this example highlights any implications when incrementing or decrementing dates, it seems to work exactly as expected. I think it only affects dates around the switchover date. For example:

d = Date.new(1582, 10, 15) # => #<Date: 1582-10-15>
d.prev_day # => #<Date: 1582-10-04>

Date.new(1582, 10, 14) # => Date::Error: invalid date

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how this example highlights any implications when incrementing or decrementing dates, it seems to work exactly as expected. I think it only affects dates around the switchover date. For example:

d = Date.new(1582, 10, 15) # => #<Date: 1582-10-15>
d.prev_day # => #<Date: 1582-10-04>

Date.new(1582, 10, 14) # => Date::Error: invalid date

Works exactly as you and I expect. Naive user may be surprised that in/de/crementing from one calendar may lead to the other.

Also, I have not illustrated the error you cite (trying to create into the jump); should we include that?

Copy link
Member

Choose a reason for hiding this comment

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

Right, but the documentation above it says

For a Date object created with Date::JULIAN or Date::GREGORIAN,
there are implications when the date is incremented or decremented

I see no implications in the examples that follow. The call to Date#next_year returns a date with the same month and day in the following year and the call to Date#prev_year returns a date with the same month and day in the previous year. Everything works exactly as expected. So I'm a bit confused as to what the examples are trying to show.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very well; I've removed them.

@BurdetteLamar
Copy link
Member Author

BurdetteLamar commented Aug 4, 2022

Can you also add the doc directory into the .document file so that it's included when generating RDoc.

Done.

We also need to add to ruby/tool/sync_default_gems.rb.

.document Outdated Show resolved Hide resolved
Copy link
Member

@peterzhu2118 peterzhu2118 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!

@peterzhu2118
Copy link
Member

I think we should update ruby/tool/sync_default_gems.rb before merging this PR so that it syncs this PR to ruby/ruby.

Co-authored-by: Peter Zhu <[email protected]>
@BurdetteLamar BurdetteLamar merged commit 9906677 into ruby:master Aug 5, 2022
Comment on lines +7 to +15
may matter to your program if it uses dates in the interval:

- October 15, 1582.
- September 14, 1752.

A date outside that interval (including all dates in modern times)
is the same in both calendars.
However, a date _within_ that interval will be different
in the two calendars.
Copy link
Member

Choose a reason for hiding this comment

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

This description is totally wrong.

This "interval" is the duration a date is represented differently across countries.
Gregorian calendar has never match Julian calendar since its creation (except for 3C in proleptic Gregorian calendar).
If the two calendars were match since 16C or 19C, why they needed switch it?

Comment on lines +28 to +30
When your code uses a date in this "gap" interval,
it will matter whether it considers the switchover date
to be the earlier date or the later date (or neither).
Copy link
Member

Choose a reason for hiding this comment

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

I think this sentence will need to refine too.

@BurdetteLamar
Copy link
Member Author

Thanks, @nobu. I will look into these things.

{Julian and Gregorian calendars}[rdoc-ref:calendars.rdoc@Julian+and+Gregorian+Calendars]
by accepting an optional argument +start+, whose value may be:

- Date::ITALY (the default): the created date is Julian
Copy link
Member

Choose a reason for hiding this comment

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

These constants are no longer links.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that, too. Should RDoc have linked to the constants?

Copy link
Member

@nobu nobu Aug 15, 2022

Choose a reason for hiding this comment

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

Before this commit, that is inside for class Date, these constants were linked properly.

Copy link
Member

Choose a reason for hiding this comment

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

This seems a bug of RDoc.
This off-page constant also isn't rendered as a link.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I link manually, as a stop-gap?

@nobu
Copy link
Member

nobu commented Aug 8, 2022

@BurdetteLamar I removed obviously wrong descriptions for now. Feel free to improve.

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

Successfully merging this pull request may close these issues.

3 participants