-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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.
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
pre-commit can be done in a separate PR actually, don't worry about it here |
Passed files to be excluded from search as seperate arguments
Perhaps it makes sense to filter the search for os.remove to specifically exclude the cases where it occurs currently, rather than exclude the whole files? If so any suggestions on preferred ways to achieve this? |
ci/code_checks.sh
Outdated
@@ -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/ |
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 we need to escape the .
in os.remove
?
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 so? os.remove
is detected without escaping the .
when files containing it are not excluded
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.
OK but like this I think it'll also detect e.g. oszremove
. You'll need to escape the .
else it'll match anything
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.
Sorry, got ya. Changed.
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.
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"?
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.
Makes sense, I've shifted it up to the 'patterns' section
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.
unless there's a better way of excluding exceptions than excluding entire files, this looks good to me 👍
thanks @jnecus |
I have included a check for instances of os.remove in the tests folder. This is in line with issue #37095 whereby ensure_clean should be used rather than os.remove.
A few outstanding questions remain:
Should this be expanded to search for cases of os.remove elsewhere?
Cases of(solved by passing arguments separately rather than as a list)os.remove
still exist in common.py, test_writers.py, and test_store.py. For some reason, the case in test_store.py is not picked up by this check? Any idea why?Advice on how to implement this as a pre-commit hook also welcome.(to be implemented in separate PR)closes Add CI check to check that
ensure_clean
is used instead ofos.remove
#37095tests added / passed
passes
black pandas
passes
git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry