gh-108638: Fix stat.filemode() when _stat is missing - #108639
Conversation
Sorry, something went wrong.
Change the pure Python implementation of stat.filemode() for unknown file type: use "?", as done by the _stat.filemode(). test_stat skips TestFilemodeCStat if the _stat extension is missing.
|
I don't think that this change should be backported to Python 3.11 and 3.12: the _stat is always built on these versions. Adopting the test suite for PyPy is nice, but I don't think that it's worth it to backport the change. |
Sorry, something went wrong.
| break | ||
| else: | ||
| perm.append("-") | ||
| if index == 0: |
There was a problem hiding this comment.
| if index == 0: | |
| if not perm: |
And you can remove index.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh right, feel free to write a follow-up PR if you want, sadly you reviewed the change just after it got merged automatically. I have not preference between enumerate() + if index == 0 and not perm.
Sorry, something went wrong.
There was a problem hiding this comment.
_stat.filemode() and stat.filemode() implementations are very different. I tried to write a minimum fix.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, so there is a _stat.filemode()? I noticed that the Python implementation always use constants defined in Python, even if they are overwritten by _stat later. I was not sure whether this is good or bad. On one hand, the original use case was for tar files, which can came from other systems. On other hand, it should work with local files.
Sorry, something went wrong.
|
This PR feels like a good example of why you should wait a bit longer for reviews before merging PRs. Also please allow time for folks in different timezones to review before merging. Waiting a few days with a minor fix like this isn't the end of the world right? |
Sorry, something went wrong.
|
Do you mean that you want to change the implémentation? |
Sorry, something went wrong.
|
No, I mean that there was a comment with a suggestion that you could have taken into account before merging, rather than saying "oops I merged already go make your own PR if you want to". This sounds like you were too hasty merging. |
Sorry, something went wrong.
|
It is just a coincidence. I started to write a review when the PR was not yet merged, but when I published my comment it was already merged. This has happened several times already, and not only with Victor's PRs. The longer the PR waits, the less likely it is to happen. It's not a big problem if it doesn't happen all the time. In this case, it was just a little style suggestion, and it's up to Victor to accept it or not. |
Sorry, something went wrong.
Change the pure Python implementation of stat.filemode() for unknown file type: use "?", as done by the _stat.filemode().
test_stat skips TestFilemodeCStat if the _stat extension is missing.