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: Updating capitalization of doc/source/development #33121

Merged
merged 36 commits into from Apr 3, 2020
Merged

DOC: Updating capitalization of doc/source/development #33121

merged 36 commits into from Apr 3, 2020

Conversation

cleconte987
Copy link
Contributor

Regarding issue #32550. Changes to documentation folder doc/source/development to capitalise title strings and keep keyword exceptions as is

…t_modification

Merging new changes from pandas-dev/pandas repository into script_modification to be up-to-date
Merge remote-tracking branch 'upstream_main_pandas/master' into script_modification
… add elements to exceptions's set in scripts/validate_rst_title_capitalization.py
Merge branch 'script_modification' into changes_to_pandas_remote
Merge remote-tracking branch 'upstream_main_pandas/master' into changes_to_pandas_remote
@@ -18,7 +18,7 @@ consistent code format throughout the project. For details see the
Patterns
========

foo.__class__
Foo.class
Copy link
Member

Choose a reason for hiding this comment

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

Actually this is on purpose, does validate_rst_title_capitalization.py showing this as an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is showing this as an error, because foo is not inside exceptions.
But moreover, it is removing underscores from the title here:
with open(rst_file, "r") as fd: previous_line = "" for i, line in enumerate(fd): line = line[:-1] line_chars = set(line) if ( len(line_chars) == 1 and line_chars.pop() in symbols and len(line) == len(previous_line) ): **yield re.sub(r"[{backtick}\*_]", "", previous_line)**, i previous_line = line

Copy link
Member

Choose a reason for hiding this comment

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

@cleconte987 can you change the title to Using foo.__class__ instead? And below same for Using f-strings.

This will make the validation pass, and things look probably even better, without adding extra complexity.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that!

@@ -95,7 +95,7 @@ on :ref:`ecosystem.extensions`.

The interface consists of two classes.

:class:`~pandas.api.extensions.ExtensionDtype`
Class:~pandas.API.extensions.ExtensionDtype
Copy link
Member

Choose a reason for hiding this comment

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

:class: is something that we use for sphinx, I don't think that this is something we want

@datapythonista
Copy link
Member

Thanks @cleconte987. Most of this is duplicated of #32944, we're going to merge that one, so you may want to revert those files. Also, try to coordinate with @themien who is also working on this.

For the titles that are like :class:... we can't apply your changes, that is a special encoding to automatically create links. What we'll have to do is to update the script to skip titles that start with :.

@datapythonista datapythonista changed the title Changes to documentation folder doc/source/development || issue #32550 DOC: Updating capitalization of doc/source/development Mar 29, 2020
@cleconte987
Copy link
Contributor Author

cleconte987 commented Apr 1, 2020

Thanks @cleconte987. Most of this is duplicated of #32944, we're going to merge that one, so you may want to revert those files. Also, try to coordinate with @themien who is also working on this.

For the titles that are like :class:... we can't apply your changes, that is a special encoding to automatically create links. What we'll have to do is to update the script to skip titles that start with :.

Ok I will follow what @themien is doing.
I modify the script as you said
And I update PR soon

…r doc/source/development

Updating script validate_rst_title_capitalization.py to avoid taking into account titles beginning with ':'
…ITALIZATION_EXCEPTIONS' that are present in title with special encoding, as it is not necessary anymore
Comment on lines 235 to 236
if ":class" in title:
print(title)
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 should be:

if ":class:" in title:
   continue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Horrible... I forgot to remove it. Correcting it right away!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm Im not so much used to git yet.. Apart from that mistake, I didn't roll back previous changes that I made, I thought it will be ok. I don't know if the changes i didn't remove will fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or if it conflicts with previous PR or something

@@ -121,6 +175,7 @@ def find_titles(rst_file: str) -> Generator[Tuple[str, int], None, None]:
len(line_chars) == 1
and line_chars.pop() in symbols
and len(line) == len(previous_line)
and previous_line[0] != ":"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think I suggested this approach myself, but I see a problem now. This will skip the whole line from validation, and I see we use a mix of these :class: directives and regular text in some titles.

Instead of skipping the line here, I think we should update the condition in line 139. So, we check that word is not one of the exceptions, and also, it doesn't start with :. We could use a regex probably, but let's start simple, and see if it's enough.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that:

  • First of all this part of code line 180

    yield re.sub(r"[`*_]", "", previous_line), i

    removes "`" from the title. So when function checks if title is correct, it will already be modified. So this is the main problem.

  • Secondly, line 129:

    correct_title: str = re.sub(r"^\W*", "", title).capitalize()

    removes all non word characters at the beginning from the desired correct title that we want to keep, so ":" is not kept as being part of the correct title

  • Thirdly, this line, number 136

    word_list = re.split(r"\W", removed_https_title)

    removes all non word characters from the list to analyse. So ":" don't appear anymore.

By the way, code in line 180 causes problems because it also removes "_" which is present in documentation like > code_style.rst line 21

Using foo.__class__

So what I suggest is to modify in script "validate_rst_title_capitalization.py" from line 180 on:

NB: I replace "`" by {backtick} as it causes trouble to display it as code

yield re.sub(r"[{backtick}\*_]", "", previous_line), i

to:

if previous_line[0] != ":": yield re.sub(r"[{backtick}\*_]", "", previous_line), i else: yield re.sub(r"[\*_]", "", previous_line), i

in order to keep the "`" if title begins by a ":"
And to add in "validate_rst_title_capitalization.py" line 126:

if title[0] == ":": return title

So that if title begins by ":" it considers title to be valid no matter what.

Or to simply add in "validate_rst_title_capitalization.py" line 126:

if title[0] == ":": return title

for the same reason.

Note that there is still a problem with:

yield re.sub(r"[`*_]", "", previous_line), i

That doesn't keep "_" in the titles

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Ok, let's fix it this way then, and we can find a better solution in the future.

Thanks for the work on this @cleconte987

@simonjayhawkins
Copy link
Member

@cleconte987 can you merge upstream master into your branch to resolve merge conflicts. see https://pandas.pydata.org/docs/development/contributing.html#updating-your-pull-request

@pep8speaks
Copy link

pep8speaks commented Apr 2, 2020

Hello @cleconte987! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-02 21:13:35 UTC

@@ -327,7 +327,7 @@ if [[ -z "$CHECK" || "$CHECK" == "docstrings" ]]; then
RET=$(($RET + $?)) ; echo $MSG "DONE"

MSG='Validate correct capitalization among titles in documentation' ; echo $MSG
$BASE_DIR/scripts/validate_rst_title_capitalization.py $BASE_DIR/doc/source/development/contributing.rst
$BASE_DIR/scripts/validate_rst_title_capitalization.py $BASE_DIR/doc/source/development/contributing.rst $BASE_DIR/doc/source/reference
Copy link
Member

Choose a reason for hiding this comment

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

If everything in development/ is fixed, can you change this in a new PR, so the whole directory is validated in the CI? Thanks!

Suggested change
$BASE_DIR/scripts/validate_rst_title_capitalization.py $BASE_DIR/doc/source/development/contributing.rst $BASE_DIR/doc/source/reference
$BASE_DIR/scripts/validate_rst_title_capitalization.py $BASE_DIR/doc/source/development $BASE_DIR/doc/source/reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@datapythonista datapythonista merged commit e17467e into pandas-dev:master Apr 3, 2020
@datapythonista
Copy link
Member

Thanks for working on this @cleconte987

@cleconte987
Copy link
Contributor Author

You're welcome!

@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants