Skip to content
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

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

Merged
merged 6 commits into from
Oct 16, 2020

Conversation

jnecus
Copy link
Contributor

@jnecus jnecus commented Oct 13, 2020

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 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? (solved by passing arguments separately rather than as a list)

  • 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 of os.remove #37095

  • tests added / passed

  • passes black pandas

  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

  • whatsnew entry

@jnecus jnecus changed the title Update code_checks.sh to check of instances of os.remove #37095 Update code_checks.sh to check for instances of os.remove #37095 Oct 13, 2020
@jnecus jnecus changed the title Update code_checks.sh to check for instances of os.remove #37095 Update code_checks.sh to check for instances of os.remove Oct 13, 2020
@MarcoGorelli
Copy link
Member

MarcoGorelli commented Oct 13, 2020

--exclude doesn't take a list, I think you'll have to pass multiple arguments

 --exclude 'common.py' --exclude 'test_writers.py' 

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
@jnecus
Copy link
Contributor Author

jnecus commented Oct 13, 2020

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?

@@ -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

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

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.

unless there's a better way of excluding exceptions than excluding entire files, this looks good to me 👍

@jbrockmendel jbrockmendel added CI Continuous Integration Code Style Code style, linting, code_checks labels Oct 14, 2020
@jreback jreback merged commit a08e1f8 into pandas-dev:master Oct 16, 2020
@jreback
Copy link
Contributor

jreback commented Oct 16, 2020

thanks @jnecus

@jnecus jnecus deleted the os-remove-code-check branch October 16, 2020 09:13
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Oct 26, 2020
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI check to check that ensure_clean is used instead of os.remove
5 participants