-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
Updated code_checks.sh with PR06 #43511
Conversation
Varun270
commented
Sep 11, 2021
- [ ✔] closes Fix PR06 issues in docstrings #28724
- tests added / passed
- [✔ ] Ensure all linting tests pass, see here for how to run them
- whatsnew entry
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.
you just need to add PR06, no need to remove any other codes / add blank lines
Actually the PR was having merge conflicts so I resolved one, maybe during this process it might have add any kind of space. |
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.
please address #43511 (review)
4460d19
to
d3bd21b
Compare
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.
Why did you remove GL02?
Yeah, I really don't know how did that happen. I will fix this asap. |
d3bd21b
to
733456a
Compare
ci/code_checks.sh
Outdated
@@ -89,7 +89,7 @@ fi | |||
### DOCSTRINGS ### | |||
if [[ -z "$CHECK" || "$CHECK" == "docstrings" ]]; then | |||
|
|||
MSG='Validate docstrings (GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS04, SS05, PR03, PR04, PR05, PR10, EX04, RT01, RT04, RT05, SA02, SA03)' ; echo $MSG | |||
MSG='Validate docstrings (GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS04, SS05, PR03, PR04, PR05, PR06, 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,PR10,EX04,RT01,RT04,RT05,SA02,SA03 |
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.
need to add PR06 to this line as well
733456a
to
fcce72d
Compare
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.
Looks like there's at least a couple of PR06 errors which still need fixing up
Error: /home/runner/work/pandas/pandas/pandas/core/tools/numeric.py:27:PR06:pandas.to_numeric:Parameter "downcast" type should use "int" instead of "integer"
Error: /home/runner/work/pandas/pandas/pandas/core/arrays/sparse/array.py:248:PR06:pandas.arrays.SparseArray:Parameter "kind" type should use "int" instead of "integer"
Here should I change integer to int in both the list and default as well? |
For the sparse case, "integer" means something different than "int" (is referring to the type of index to construct, not a datatype), so I don't think it should change. Maybe this is a false positive? |
hmm, yeah, this might be a bug in numpydoc..I'll get back to this, might be worth checking if it also happens with the latest release |
Similar thing with downcast parameter, What should I do here @mzeitlin11 ? |
You could look into what @MarcoGorelli was saying above - whether this is an issue with |
Till then Can I work on issue#21749 |
yes, of course! |
Please do assign it to me as the bot is not responding to the "take" command. |
multiple people can work on that issue, there's no need to be assigned |
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
@MarcoGorelli This issue has been labeled stale due to inactivity. I was just wondering if there's anything that I have to do or you are handling it from your side. |
Thanks @Varun270 - I haven't had a chance to look at this yet, I'll have a look at whether it needs reporting to NumpyDoc |
I reported this to NumpyDoc in numpy/numpydoc#341 Perhaps we can just work around it with something like
until/if it's fixed upstream |
So should I create a PR to add the changes that you suggested? |
you can update this one |
alright will do it |
@MarcoGorelli I pushed the changes can you tell me what's wrong? |
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.
You'll also need to fix downcast
(and fix the merge conflicts)
pandas/core/tools/numeric.py
Outdated
@@ -48,7 +48,7 @@ def to_numeric(arg, errors="raise", downcast=None): | |||
- If 'raise', then invalid parsing will raise an exception. | |||
- If 'coerce', then invalid parsing will be set as NaN. | |||
- If 'ignore', then invalid parsing will return the input. | |||
downcast : {'integer', 'signed', 'unsigned', 'float'}, default None | |||
downcast : {'int', 'signed', 'unsigned', 'float'}, default None |
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.
needs to be a fix similar to what you did in pandas/core/arrays/sparse/array.py, this won't work
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.
Okay you mean something like this -
downcast : str
Can be 'integer' or 'signed' or 'unsigned' or 'float', default is 'None'.
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'd suggest
downcast : str, default None
Can be 'integer', 'signed', 'unsigned', or 'float'.
ci/code_checks.sh
Outdated
MSG='Validate docstrings (GL01, GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS03, SS04, SS05, PR03, PR04, PR05, PR08, PRO9, PR10, EX04, RT01, RT04, RT05, SA02, SA03)' ; echo $MSG | ||
$BASE_DIR/scripts/validate_docstrings.py --format=actions --errors=GL01,GL02,GL03,GL04,GL05,GL06,GL07,GL09,GL10,SS02,SS03,SS04,SS05,PR03,PR04,PR05,PR08,PR09,PR10,EX04,RT01,RT04,RT05,SA02,SA03 | ||
MSG='Validate docstrings (GL02, GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS01, SS02, SS04, SS05, PR03, PR04, PR05, PR06, 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,PR06,PR10,EX04,RT01,RT04,RT05,SA02,SA03 |
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 don't think you did the merge quite correctly - can you try again, and make sure that you only add PR06 (and not remove anything else, nor add any blank lines)?
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.
Do you mean I have to add GL01, SS03, PR08, and PR09? Since PR06 is already added.
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.
the diff shows that your PR removes GL01, SS03, PR08, and PR09 - these should not be removed, the only thing this PR should do to this file is to add PR06
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.
Done!!
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.
thanks @Varun270 |
looks like this wasn't green
|