Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-133503: Updatecompileall.rst's documentation of-s and-p for clarity,#134756

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
sharktide wants to merge6 commits intopython:main
base:main
Choose a base branch
Loading
fromsharktide:patch-133503

Conversation

sharktide
Copy link
Contributor

@sharktidesharktide commentedMay 26, 2025•
edited by github-actionsbot
Loading

This willclose#133503

This PR simple updatescompileall.rst to alert users to these following risks as stated by@mgorny

Removing the prefix makes paths relative to it.
-s and -p can be used simultaneously.
-p / can be used to make the path absolute.

📚 Documentation preview 📚:https://cpython-previews--134756.org.readthedocs.build/

Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

Instead of having a common entry for-s and-p, we should have different entries as the warning (which should not be a warning IMO) is only for-s.

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@sharktide
Copy link
ContributorAuthor

@picnixz IMO it should be a warning considering it could cause breaking changes to programs if people don't pay close attention

@sharktide
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-app
Copy link

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-appbedevere-appbot requested a review frompicnixzMay 26, 2025 23:37
@picnixz
Copy link
Member

IMO it should be a warning considering it could cause breaking changes to programs if people don't pay close attention

I don't see how it's a breaking change. This has been the default behavior for ages. And warnings boxes should be used carefully as they disrupt the reading flow.

@sharktide
Copy link
ContributorAuthor

Ok! I am doing it. Please review in a minute

@picnixz
Copy link
Member

I'll review tomorrow. It's 2 AM for me

@sharktide
Copy link
ContributorAuthor

sharktide commentedMay 26, 2025•
edited
Loading

Sorry! It's only 19:52 for me

*I love 24 hr time even though I live in the US*

@sharktide
Copy link
ContributorAuthor

@picnixz Get some sleep and please review in your morning :)

Copy link
Contributor

@mgornymgorny left a comment

Choose a reason for hiding this comment

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

I think the documentation is inconsistent now. Previously-s and-p were documented as a single paragraph following both options. Now-s is followed by a comment, then-p is followed by the description of both options. They should either be described separately, or both notes need to be placed below-p.

Copy link
Member

@picnixzpicnixz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

I would suggest the following:

..option::-sstrip_prefix   Remove the given prefix from paths recorded in the ``.pyc`` files.   Paths are made relative to the prefix.   This option can be used with ``-p`` but not with ``-d``...option::-pprepend_prefix   Append the given prefix to paths recorded in the ``.pyc`` files.   Use ``-p /`` to make the paths absolute.   This option can be used with ``-s`` but not with ``-d``.

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@sharktide
Copy link
ContributorAuthor

@picnixz Done. Review when you can, please.

@sharktide
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-app
Copy link

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-appbedevere-appbot requested a review frompicnixzMay 27, 2025 11:59
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@sharktide
Copy link
ContributorAuthor

@picnixz Fixed it

@sharktide
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-app
Copy link

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

@bedevere-appbedevere-appbot requested a review frompicnixzMay 27, 2025 16:28
Copy link
Contributor

@mgornymgorny left a comment

Choose a reason for hiding this comment

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

I think it's clear enough for me.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mgornymgornymgorny approved these changes

@picnixzpicnixzAwaiting requested review from picnixz

Assignees
No one assigned
Labels
awaiting change reviewdocsDocumentation in the Doc dirskip news
Projects
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

compileall:-s surprisingly makes path relative
3 participants
@sharktide@picnixz@mgorny
[8]ページ先頭
©2009-2025 Movatter.jp