GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-108638: Fix stat.filemode() when _stat is missing by vstinner · Pull Request #108639 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Aug 29, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: Test suite expects _stat extension to be available #108638

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot mentioned this pull request Aug 29, 2023
Test suite expects _stat extension to be available #108638
Closed
2 tasks done
@bedevere-bot bedevere-bot added the awaiting core review label Aug 29, 2023
@vstinner
pythongh-108638: Fix stat.filemode() when _stat is missing
ce5c938 
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.
@vstinner
vstinner force-pushed the filemode branch from 74062c3 to ce5c938 Compare August 29, 2023 14:57
@vstinner vstinner added the skip news label Aug 29, 2023
@vstinner

vstinner commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner enabled auto-merge (squash) August 29, 2023 15:44
@vstinner
vstinner merged commit b62a760 into python:main Aug 29, 2023
@bedevere-bot bedevere-bot removed the awaiting core review label Aug 29, 2023
serhiy-storchaka
serhiy-storchaka reviewed Aug 29, 2023
View reviewed changes
Comment thread Lib/stat.py
break
else:
perm.append("-")
if index == 0:

@serhiy-storchaka serhiy-storchaka Aug 29, 2023

Copy link
Copy Markdown
Member

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.

Suggested change
if index == 0:
if not perm:

And you can remove index.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Aug 29, 2023

Copy link
Copy Markdown
Member Author

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.

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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner Aug 29, 2023

Copy link
Copy Markdown
Member Author

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.

_stat.filemode() and stat.filemode() implementations are very different. I tried to write a minimum fix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Aug 30, 2023

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
vstinner deleted the filemode branch August 29, 2023 15:51
@gvanrossum

gvanrossum commented Aug 29, 2023

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Aug 29, 2023

Copy link
Copy Markdown
Member Author

Do you mean that you want to change the implémentation?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Aug 29, 2023

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Aug 30, 2023

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vstinner @gvanrossum @serhiy-storchaka @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.