Skip to content

Update code_checks.sh to check for instances of os.remove #37097

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.

Sign up for GitHub

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 6 commits into from
Oct 16, 2020
Merged

Update code_checks.sh to check for instances of os.remove #37097

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee7a7ba
Update code_checks.sh
jnecus Oct 13, 2020
95d3ee6
Merge remote-tracking branch 'upstream/master' into os-remove-code-check
jnecus Oct 13, 2020
c553d11
Update code_checks.sh
jnecus Oct 13, 2020
bdab538
Merge remote-tracking branch 'upstream/master' into os-remove-code-check
jnecus Oct 14, 2020
fa820cd
Update code_checks.sh
jnecus Oct 14, 2020
aee1a9e
Update code_checks.sh
jnecus Oct 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ci/code_checks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,10 @@ if mods:
"
RET=$(($RET + $?)) ; echo $MSG "DONE"

MSG='Check code for instances of os.remove' ; echo $MSG
invgrep -R --include="*.py*" --exclude "common.py" --exclude "test_writers.py" --exclude "test_store.py" -E "os.remove" pandas/tests/
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to escape the . in os.remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so? os.remove is detected without escaping the . when files containing it are not excluded

Copy link
Member

@MarcoGorelli MarcoGorelli Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK but like this I think it'll also detect e.g. oszremove. You'll need to escape the . else it'll match anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, got ya. Changed.

Copy link
Member

Choose a reason for hiding this comment

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

cool, thanks - I'm not sure about the location of the check though, as it's currently inside the "checks on imported code" section. Should it be in "looking for unwanted patterns"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I've shifted it up to the 'patterns' section

RET=$(($RET + $?)) ; echo $MSG "DONE"

fi

### DOCTESTS ###
Expand Down