Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

replace os.path.getsize with Path("file.txt").stat().st_size #189

Merged
merged 4 commits into from
Jan 30, 2023

Conversation

doomedraven
Copy link
Contributor

Hello is my first try to extend refurb. if i did something wrong let me know please.
The ID is 154 bcz latest is 153 (according to grep -r "code = " .| cut -d"=" -f 2|sort|uniq)

Hello is my first try to extend refurb. if i did something wrong let me know please. the ID is 154 bcz latest is 153 (according to `grep -r "code = " .| cut -d"=" -f 2|sort|uniq`)
Copy link
Owner

@dosisod dosisod left a comment

Choose a reason for hiding this comment

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

Thank you @doomedraven for this PR! Everything looks good for the most part, minus the comments I left.

In addition, there are no tests for this code yet. I need to update the documentation for this, but in short, all you have to do is create a new file named test/data/err_xxx.py (where xxx is the error id), and add some code which will emit an error:

from os.path import getsize

_ = getsize("filename")

Then, run make update-tests. This will create a file called test/data/err_xxx.txt with content similar to:

test/data/err_xxx.py:3:5 [FURBxxx]: Replace `os.path.getsize(x)` with `Path(x).stat().st_size`

Note that make update-tests only runs if the .py test file was changed, not if the check itself was updated. If you update the check but not the test file, you will have to run this instead:

refurb test/data/err_xxx.py --quiet > test/data/err_xxx.txt

I appreciate you putting in the effort to make a PR for Refurb, and for the most part, this is ready to be merged once the other notes are addressed!

(And sorry for the delay on this review, I have been on vacation for the past week 🌴)

@doomedraven
Copy link
Contributor Author

thank you for feedback. i got a question. Generated test shows FURB146, shouldn't it be FURB155?
test/data/err_155.py:4:5 [FURB146]:

Copy link
Owner

@dosisod dosisod left a comment

Choose a reason for hiding this comment

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

Interesting, when I run it on my machine I get 155. I can't see why it would be doing that, but if it is still doing that let me know and I will look into it further.

Also, the newly added os.path.getatime related functions are not showing up in the tests. When debugging, they are showing up as genericpath.getatime, so we probably need to expand these out like you did for the first one (ie, adding genericpath, posixpath, and ntpath). Doing that for each one might get tedious, so we might want to clean up/normalize the path name before we do the lookup.

@doomedraven
Copy link
Contributor Author

about 146 was because I had added check size to my refurb to check my code. Thanks for feedback

@dosisod
Copy link
Owner

dosisod commented Jan 29, 2023

To fix the failing tests, add another line to the err_155.py file:

os.path.getsize(__file__)

And run make update-tests. You don't have to add this for every os.path function, just one will do!

Copy link
Owner

@dosisod dosisod left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@dosisod dosisod merged commit 8fbf342 into dosisod:master Jan 30, 2023
@doomedraven doomedraven deleted the patch-1 branch January 30, 2023 20:34
@doomedraven
Copy link
Contributor Author

thanks for help to get this merged

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants