Skip to content

validate PR09 errors #44524

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

Conversation

soumyasomasundaran
Copy link
Contributor

  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry

@MarcoGorelli MarcoGorelli added this to the 1.4 milestone Nov 19, 2021
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks @soumyas567 , looks good to me pending green

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Actually, looks like there's still a few of these validations which need fixing up:

Error: /home/runner/work/pandas/pandas/pandas/core/frame.py:7703:PR09:pandas.DataFrame.groupby:Parameter "dropna" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/core/generic.py:10955:PR09:pandas.DataFrame.rolling:Parameter "closed" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:82:PR09:pandas.io.formats.style.Styler:Parameter "escape" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:786:PR09:pandas.io.formats.style.Styler.format:Parameter "decimal" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:786:PR09:pandas.io.formats.style.Styler.format:Parameter "thousands" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:983:PR09:pandas.io.formats.style.Styler.format_index:Parameter "decimal" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:983:PR09:pandas.io.formats.style.Styler.format_index:Parameter "thousands" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:3136:PR09:pandas.io.formats.style.Styler.highlight_quantile:Parameter "color" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:2760:PR09:pandas.io.formats.style.Styler.bar:Parameter "props" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:466:PR09:pandas.io.formats.style.Styler.to_latex:Parameter "position" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/parquet.py:437:PR09:pandas.read_parquet:Parameter "path" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/sql.py:330:PR09:pandas.read_sql_query:Parameter "dtype" description should finish with "."
Error: None:None:PR09:pandas.Timestamp:Parameter "fold" description should finish with "."
Error: None:None:PR09:pandas.Timestamp.max:Parameter "fold" description should finish with "."
Error: None:None:PR09:pandas.Timestamp.min:Parameter "fold" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/core/series.py:1787:PR09:pandas.Series.groupby:Parameter "dropna" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/core/generic.py:10955:PR09:pandas.Series.rolling:Parameter "closed" description should finish with "."

Do you want to add those full stops, so that the validation script passes?

@MarcoGorelli MarcoGorelli removed this from the 1.4 milestone Nov 19, 2021
@soumyasomasundaran
Copy link
Contributor Author

Actually, looks like there's still a few of these validations which need fixing up:

Error: /home/runner/work/pandas/pandas/pandas/core/frame.py:7703:PR09:pandas.DataFrame.groupby:Parameter "dropna" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/core/generic.py:10955:PR09:pandas.DataFrame.rolling:Parameter "closed" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:82:PR09:pandas.io.formats.style.Styler:Parameter "escape" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:786:PR09:pandas.io.formats.style.Styler.format:Parameter "decimal" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:786:PR09:pandas.io.formats.style.Styler.format:Parameter "thousands" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:983:PR09:pandas.io.formats.style.Styler.format_index:Parameter "decimal" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style_render.py:983:PR09:pandas.io.formats.style.Styler.format_index:Parameter "thousands" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:3136:PR09:pandas.io.formats.style.Styler.highlight_quantile:Parameter "color" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:2760:PR09:pandas.io.formats.style.Styler.bar:Parameter "props" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/formats/style.py:466:PR09:pandas.io.formats.style.Styler.to_latex:Parameter "position" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/parquet.py:437:PR09:pandas.read_parquet:Parameter "path" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/io/sql.py:330:PR09:pandas.read_sql_query:Parameter "dtype" description should finish with "."
Error: None:None:PR09:pandas.Timestamp:Parameter "fold" description should finish with "."
Error: None:None:PR09:pandas.Timestamp.max:Parameter "fold" description should finish with "."
Error: None:None:PR09:pandas.Timestamp.min:Parameter "fold" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/core/series.py:1787:PR09:pandas.Series.groupby:Parameter "dropna" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/core/generic.py:10955:PR09:pandas.Series.rolling:Parameter "closed" description should finish with "."

Do you want to add those full stops, so that the validation script passes?

Yes I will do that.

@soumyasomasundaran
Copy link
Contributor Author

I have added full stops. But Sorry not able to find 5 of them.

Don't know where to look for the following.
Error: None:None:PR09:pandas.Timestamp:Parameter "fold" description should finish with "."
Error: None:None:PR09:pandas.Timestamp.max:Parameter "fold" description should finish with "."
Error: None:None:PR09:pandas.Timestamp.min:Parameter "fold" description should finish with "."

could not find a description for parameter 'closed' in the particular file.
Error: /home/runner/work/pandas/pandas/pandas/core/generic.py:10955:PR09:pandas.Series.rolling:Parameter "closed" description should finish with "."
Error: /home/runner/work/pandas/pandas/pandas/core/generic.py:10955:PR09:pandas.DataFrame.rolling:Parameter "closed" description should finish with "."

@phofl
Copy link
Member

phofl commented Nov 19, 2021

First three are in timestamps.pyx

@phofl
Copy link
Member

phofl commented Nov 19, 2021

The others are probably in rolling.py in for class Window

@MarcoGorelli
Copy link
Member

Yeah, groupby is here

dropna : bool, default True
If True, and if group keys contain NA values, NA values together
with row/column will be dropped.
If False, NA values will also be treated as the key in groups

Window:

closed : str, default None
If ``'right'``, the first point in the window is excluded from calculations.
If ``'left'``, the last point in the window is excluded from calculations.
If ``'both'``, the no points in the window are excluded from calculations.
If ``'neither'``, the first and last points in the window are excluded
from calculations.
Default ``None`` (``'right'``)
.. versionchanged:: 1.2.0
The closed parameter with fixed windows is now supported.

Admittedly these aren't the easiest to find

@MarcoGorelli MarcoGorelli added this to the 1.4 milestone Nov 19, 2021
Copy link
Member

@MarcoGorelli MarcoGorelli 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, thanks @soumyas567 !

cc @attack68 in case you have comments on the to_latex change

@@ -504,7 +504,7 @@ def to_latex(
position : str, optional
The LaTeX positional argument (e.g. 'h!') for tables, placed in location:

\\begin{table}[<position>]
\\begin{table}[<position>].
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this because this line is structured to reflect a console output and shouldn't include the period. The colons are the punctuation indicating a figure that follows (even if the figure is text)

Copy link
Member

@MarcoGorelli MarcoGorelli Nov 20, 2021

Choose a reason for hiding this comment

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

Perhaps it should be written as:

``\begin{table}[<position>]``.

(with 2 backticks before and after) then, to make clear that the full stop isn't part of the console output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Sure I will do it.

@attack68
Copy link
Contributor

One comment made, all others are fine with me.

@jreback
Copy link
Contributor

jreback commented Nov 20, 2021

pls also merge master

@MarcoGorelli
Copy link
Member

pls also merge master

@soumyas567 in case it's not clear:

git fetch upstream
git merge upstream/master
git push origin HEAD

@soumyasomasundaran
Copy link
Contributor Author

I am not sure if the merging was correct. Hopefully, I did not mess it up. @MarcoGorelli

@lithomas1
Copy link
Member

@soumyas567 Thanks for the PR. Can you fix up the merge conflict? We'll get it merged afterwards.

@MarcoGorelli
Copy link
Member

I am not sure if the merging was correct. Hopefully, I did not mess it up. @MarcoGorelli

Yup, the merging was correct!

There have since been new changes upstream, so you'll need to pull these too:

git fetch upstream
git merge upstream/master

and then resolve the merge conflicts (if it's not something you're familiar with, check out https://youtu.be/xNVM5UxlFSA)

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

This isn't quite right - if you click "files changed", you see

<<<<<<< HEAD
    MSG='Validate docstrings (GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS04, SS05, PR03, PR04, PR05, PR09, PR10, EX04, RT01, RT04, RT05, SA02, SA03)' ; echo $MSG
    $BASE_DIR/scripts/validate_docstrings.py --format=actions --errors=GL02,GL03,GL04,GL05,GL06,GL07,GL09,GL10,SS02,SS04,SS05,PR03,PR04,PR05,PR09,PR10,EX04,RT01,RT04,RT05,SA02,SA03
=======
    MSG='Validate docstrings (GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS03, SS04, SS05, PR03, PR04, PR05, PR10, EX04, RT01, RT04, RT05, SA02, SA03)' ; echo $MSG
    $BASE_DIR/scripts/validate_docstrings.py --format=actions --errors=GL02,GL03,GL04,GL05,GL06,GL07,GL09,GL10,SS02,SS03,SS04,SS05,PR03,PR04,PR05,PR10,EX04,RT01,RT04,RT05,SA02,SA03
>>>>>>> upstream/master
    RET=$(($RET + $?)) ; echo $MSG "DONE"

I'd suggest removing the top part (between <<<<<<< HEAD and =======), adding PR09 to the lines below them, and then removing the >>>>>>> upstream/master line

@soumyasomasundaran
Copy link
Contributor Author

This isn't quite right - if you click "files changed", you see

<<<<<<< HEAD
    MSG='Validate docstrings (GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS04, SS05, PR03, PR04, PR05, PR09, PR10, EX04, RT01, RT04, RT05, SA02, SA03)' ; echo $MSG
    $BASE_DIR/scripts/validate_docstrings.py --format=actions --errors=GL02,GL03,GL04,GL05,GL06,GL07,GL09,GL10,SS02,SS04,SS05,PR03,PR04,PR05,PR09,PR10,EX04,RT01,RT04,RT05,SA02,SA03
=======
    MSG='Validate docstrings (GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS03, SS04, SS05, PR03, PR04, PR05, PR10, EX04, RT01, RT04, RT05, SA02, SA03)' ; echo $MSG
    $BASE_DIR/scripts/validate_docstrings.py --format=actions --errors=GL02,GL03,GL04,GL05,GL06,GL07,GL09,GL10,SS02,SS03,SS04,SS05,PR03,PR04,PR05,PR10,EX04,RT01,RT04,RT05,SA02,SA03
>>>>>>> upstream/master
    RET=$(($RET + $?)) ; echo $MSG "DONE"

I'd suggest removing the top part (between <<<<<<< HEAD and =======), adding PR09 to the lines below them, and then removing the >>>>>>> upstream/master line

Shall I do this from GitHub itself? I think I did "'keep both changes" while resolving conflicts . My local repository seems up-to-date now.

@MarcoGorelli
Copy link
Member

Up to you, if you want to do it from the GitHub UI or in your editor

If you do it on GitHub, make sure to then do git pull so your local branch is up-to-date

@soumyasomasundaran
Copy link
Contributor Author

Up to you, if you want to do it from the GitHub UI or in your editor

If you do it on GitHub, make sure to then do git pull so your local branch is up-to-date

Is it OK now?

Copy link
Member

@MarcoGorelli MarcoGorelli 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, let's just wait on @attack68 to confirm if the to_latex output looks alright like this

@attack68
Copy link
Contributor

lgtm

@MarcoGorelli
Copy link
Member

Thanks @soumyas567

@MarcoGorelli MarcoGorelli merged commit 8bd9f64 into pandas-dev:master Nov 23, 2021
@soumyasomasundaran
Copy link
Contributor Author

Thank you all. :) Thank you for the help @MarcoGorelli

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.

6 participants