Skip to content

Fixed all warning.warn messages for space,full-stop etc issues #43163

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

Merged
merged 8 commits into from
Aug 25, 2021

Conversation

Kavya9986
Copy link
Contributor

@Kavya9986 Kavya9986 commented Aug 22, 2021

I have fixed all the warning.warn messages in the entire code base for "Minor typo on warnings #39158" reported issue.

closes #39158

Summary of the changes .
Fix-Tracking-Sheet (2).xlsx

@pep8speaks
Copy link

pep8speaks commented Aug 22, 2021

Hello @Kavya9986! 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 2021-08-25 09:55:36 UTC

@debnathshoham
Copy link
Member

Hi @Kavya9986 - few tests are failing because the old error messages are used in them. You might want to update the error msgs as well.
You can find more details of the failed tests here.
You may also run the tests locally as mentioned in the doc.

"round-trippable.")
warnings.warn(
"Index names beginning with 'level_' are not " "round-trippable."
)
Copy link
Member

Choose a reason for hiding this comment

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

There were two " in the original because the string was split in two lines.
The new string should be "Index names beginning with 'level_' are not round-trippable." (remove the " " from the middle of string).

@Kavya9986
Copy link
Contributor Author

Hi @Kavya9986 - few tests are failing because the old error messages are used in them. You might want to update the error msgs as well.
You can find more details of the failed tests here.
You may also run the tests locally as mentioned in the doc

Hi !
Thanks. This is very useful . I will try fixing the error messages .

@MarcoGorelli MarcoGorelli self-requested a review August 24, 2021 13:25
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.

Nice - did you run a script to find all of these?

@Kavya9986
Copy link
Contributor Author

Yes . I ran a script .

@MarcoGorelli
Copy link
Member

cool, do you want to share it? perhaps a pre-commit check could be made out of it

@Kavya9986
Copy link
Contributor Author

List of files changed.
files_changed.txt

Script

#!/bin/bash

while IFS= read -r LINE; do
    echo $LINE
    pre-commit run flake8 --files "$LINE"
done < files_changed.txt

# files_changed.txt are the list of files that where changed since last commit

@MarcoGorelli
Copy link
Member

I meant, to find the warnings which needed changing - I don't think pre-commit run flake8 --files will tell you that?

@Kavya9986
Copy link
Contributor Author

Kavya9986 commented Aug 25, 2021

Oh ! Initially I tried automating it fully , but did it partially as I thought return on my time investment would not be justified .

So first I ran this command to find the files and line number where potential fix was required
grep -rn 'warnings.warn(' . | cut -d":" -f1,2 | tr ":" "," > ../fix_tracking_sheet.csv

And then filtered out all the .c files as all the warning messages there where commented .

After which I used this grep -n8 'warnings.warn(' <File Name> to do quick checks on each directory , fixed wherever required .

I'm still trying to use some kind of pattern-matching to automate the whole task :)

I meant, to find the warnings which needed changing - I don't think pre-commit run flake8 --files will tell you that?

@MarcoGorelli MarcoGorelli added this to the 1.4 milestone Aug 25, 2021
@MarcoGorelli MarcoGorelli merged commit c979bd8 into pandas-dev:master Aug 25, 2021
feefladder pushed a commit to feefladder/pandas that referenced this pull request Sep 7, 2021
…s-dev#43163)

* Fixed all warning.warn messages for space,fullstop etc issues

* fix ppe 8 issues reported

* fix ppe 8 issues reported

* fix ppe 8 issues reported

* After precommit

* fixed the expected variable

* fixed flake8
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.

Minor typo on warnings
4 participants