-
-
Notifications
You must be signed in to change notification settings - Fork 17.6k
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: Fixed validate_title_capitalization warnings in most recent what… #59146
base: main
Are you sure you want to change the base?
Conversation
…snew doc files. Sorted exceptions list alphabetically, for better maintainability, proposed name change from CAPITALIZATION_EXCEPTIONS to CAPITALIZATION_EXCLUSIONS. (pandas-dev#32550)
doc/source/whatsnew/v2.2.0.rst
Outdated
@@ -858,7 +858,7 @@ MultiIndex | |||
^^^^^^^^^^ | |||
- Bug in :meth:`MultiIndex.get_indexer` not raising ``ValueError`` when ``method`` provided and index is non-monotonic (:issue:`53452`) | |||
|
|||
I/O | |||
IO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to drop slash here, as "IO" has been already added to exception list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be left as is.
"HTML", | ||
"SAS", | ||
"SQL", | ||
CAPITALIZATION_EXCLUSIONS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposed to change the name of this list from _EXCEPTIONS to _EXCLUSIONS, because exceptions seems to be more related to error handling in the code itself and this name may be confusing.
However I'm happy to discuss or drop this change if you think it's out of scope of that story or is unnecessary
Thanks for the PR! I don't think it's that important to update old release notes' capitalization, updating other parts of the documentation is probably more important. But, this is still appreciated! |
} | ||
|
||
CAP_EXCEPTIONS_DICT = {word.lower(): word for word in CAPITALIZATION_EXCEPTIONS} | ||
CAP_EXCEPTIONS_DICT = {word.lower(): word for word in CAPITALIZATION_EXCLUSIONS} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're gonna rename CAPITALIZATION_EXCEPTIONS
to CAPITALIZATION_EXCLUSIONS
then this dict will have to be renamed to CAP_EXCLUSIONS_DICT
as well.
But, before you do that, let's wait for a pandas maintainer's opinion on renaming this. I don't want to ask you to do something just for it to be reverted in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
Alright - let's wait for a decision and I'll act basing on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to exclusions makes sense to me.
doc/source/whatsnew/v2.2.0.rst
Outdated
@@ -17,7 +17,7 @@ Upcoming changes in pandas 3.0 | |||
|
|||
pandas 3.0 will bring two bigger changes to the default behavior of pandas. | |||
|
|||
Copy-on-Write | |||
Copy-on-write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the time I've seen 'Copy on Write' being written is with a capital W. Maybe we should add this to the exceptions list
doc/source/whatsnew/v2.2.0.rst
Outdated
@@ -858,7 +858,7 @@ MultiIndex | |||
^^^^^^^^^^ | |||
- Bug in :meth:`MultiIndex.get_indexer` not raising ``ValueError`` when ``method`` provided and index is non-monotonic (:issue:`53452`) | |||
|
|||
I/O | |||
IO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be left as is.
Thank you! |
There's one error I don't know how to tackle: "pandas" in underscore is already in the list of exclusions, but here it is the beginning of the title so it is expected to be upperscore |
From the citing webpage: "When using the project name pandas, please use it in lower case, even at the beginning of a sentence." -- Not sure why this rule was made but it's quite interesting |
Alright, thanks for your response! Then it is clear. pandas should be underscore even at the beginning of the sentence |
This is generally how names are - they are styled in upper/lower case independent of other grammatical rules, and always done in a consistent manner. For example, words may start with an uppercase letter but they don't appear in the middle unless it is a name, e.g. McClain. And you would never see "IPhone" at the beginning of a sentence, it's always "iPhone". You see this with other packages as well, e.g. scikit-learn. |
…snew doc files. Sorted exceptions list alphabetically, for better maintainability, proposed name change from CAPITALIZATION_EXCEPTIONS to CAPITALIZATION_EXCLUSIONS. (#32550)
closescontributes to Fix capitalization among headings in documentation files #32550 (according to the discussion, the story would touch too many files at once, hence the reviewing process would be cumbersome for the reviewer)- [ ] Tests added and passed if fixing a bug or adding a new feature[ ] Added an entry in the latestdoc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.