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 Easter typos #1026

Merged
merged 2 commits into from
Apr 4, 2020
Merged

Fix Easter typos #1026

merged 2 commits into from
Apr 4, 2020

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Apr 4, 2020

Summary of changes

Easter is a proper noun.

Fixes https://dateutil.readthedocs.io/en/stable/easter.html

Pull Request Checklist

  • [n/a] Changes have tests
  • Authors have been added to AUTHORS.md
  • [n/a] News fragment added in changelog.d. See CONTRIBUTING.md for details

Copy link
Member

@pganssle pganssle left a comment

Choose a reason for hiding this comment

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

@hugovk Can you rebase against master? I merged a PR adding GHA like the same minute you opened this PR.

@hugovk
Copy link
Contributor Author

hugovk commented Apr 4, 2020

Yes, will include a fix for the numbered list formatting:

image

@ffe4
Copy link
Member

ffe4 commented Apr 4, 2020

Screen Shot 2020-04-04 at 16 05 04

Not sure I like this look better. Also, aren't these numbers supposed to represent the corresponding constant values, instead of just a numbered listing?

If not we could just omit the numbering, unless there are some conventions about this.

@hugovk
Copy link
Contributor Author

hugovk commented Apr 4, 2020

That's the old look: https://dateutil.readthedocs.io/en/stable/easter.html

The new look will be more like:

image

@pganssle pganssle self-requested a review April 4, 2020 14:25
@ffe4
Copy link
Member

ffe4 commented Apr 4, 2020

Sorry, I did just quickly copy-paste your code instead of pulling it via git and made a mistake. Your screenshot is correct.

@pganssle
Copy link
Member

pganssle commented Apr 4, 2020

@ffe4 I think it might be a coincidence that the numbering happens to match the values. We could maybe rearrange things to use non-numbered bullets, or to have non-numbered bullet points that mention the constant directly, but for the purposes of this minor PR we can keep it as is and just fix the numbering.

@pganssle
Copy link
Member

pganssle commented Apr 4, 2020

Appveyor status reporting is currently not working and there appears to be very little I can do about it, but the actual build is passing so I'm going to go ahead and merge.

I guess we can skip the news on this as it's pretty trivial.

@pganssle pganssle merged commit c175137 into dateutil:master Apr 4, 2020
@hugovk hugovk deleted the patch-1 branch April 4, 2020 16:43
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.

None yet

3 participants