gh-108638: Fix stat.filemode() when _stat is missing #108639
Conversation
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 |
| for bit, char in table: | ||
| if mode & bit == bit: | ||
| perm.append(char) | ||
| 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.
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.
There was a problem hiding this comment.
_stat.filemode() and stat.filemode() implementations are very different. I tried to write a minimum fix.
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.
|
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? |
|
Do you mean that you want to change the implémentation? |
|
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. |
|
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. |
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.