Skip to content

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

Merged
picnixz merged 6 commits intopython:mainfrom
sharktide:patch-133503
Jun 1, 2025
Merged

gh-133503: Update compileall.rst's documentation of -s and -p for clarity, #134756
picnixz merged 6 commits intopython:mainfrom
sharktide:patch-133503

Conversation

@sharktide
Copy link
Copy Markdown
Contributor

@sharktide sharktide commented May 26, 2025 •
edited by github-actions Bot
Loading

This will close #133503

This PR simple updates compileall.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/

@bedevere-app bedevere-app Bot mentioned this pull request May 26, 2025
Closed
@bedevere-app bedevere-app Bot added docs Documentation in the Doc dir skip news labels May 26, 2025
@github-project-automation github-project-automation Bot moved this to Todo in Docs PRs May 26, 2025
picnixz
picnixz requested changes May 26, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@picnixz picnixz 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.

Comment thread Doc/library/compileall.rst Outdated
Comment thread Doc/library/compileall.rst Outdated
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 26, 2025

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 phrase I 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
Copy Markdown
Contributor Author

sharktide commented May 26, 2025

@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
Copy Markdown
Contributor Author

sharktide commented May 26, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 26, 2025

Thanks for making the requested changes!

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

@bedevere-app bedevere-app Bot requested a review from picnixz May 26, 2025 23:37
picnixz
picnixz reviewed May 26, 2025
View reviewed changes
Comment thread Doc/library/compileall.rst Outdated
Comment thread Doc/library/compileall.rst Outdated
Comment thread Doc/library/compileall.rst Outdated
@picnixz
Copy link
Copy Markdown
Member

picnixz commented May 26, 2025

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
Copy Markdown
Contributor Author

sharktide commented May 26, 2025

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

@picnixz
Copy link
Copy Markdown
Member

picnixz commented May 26, 2025

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

@sharktide
Copy link
Copy Markdown
Contributor Author

sharktide commented May 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
Copy Markdown
Contributor Author

sharktide commented May 26, 2025

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

mgorny
mgorny suggested changes May 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@mgorny mgorny 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.

picnixz
picnixz requested changes May 27, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@picnixz picnixz 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:: -s strip_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:: -p prepend_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``.

Comment thread Doc/library/compileall.rst Outdated
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 27, 2025

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 phrase I 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.

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 27, 2025

Thanks for making the requested changes!

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

@bedevere-app bedevere-app Bot requested a review from picnixz May 27, 2025 11:59
picnixz
picnixz reviewed May 27, 2025
View reviewed changes
Comment thread Doc/library/compileall.rst Outdated
@sharktide @picnixz
pythongh-133503 Fix wrong words - Append to Prepend
812c25c 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@sharktide
Copy link
Copy Markdown
Contributor Author

sharktide commented May 27, 2025

@picnixz Fixed it

@sharktide
Copy link
Copy Markdown
Contributor Author

sharktide commented May 27, 2025

I have made the requested changes; please review again

@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented May 27, 2025

Thanks for making the requested changes!

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

@bedevere-app bedevere-app Bot requested a review from picnixz May 27, 2025 16:28
mgorny
mgorny approved these changes May 27, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@mgorny mgorny 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.

@picnixz picnixz merged commit fe6f8a3 into python:main Jun 1, 2025
27 of 28 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in Docs PRs Jun 1, 2025
@picnixz picnixz added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jun 1, 2025
@miss-islington-app
Copy link
Copy Markdown

miss-islington-app Bot commented Jun 1, 2025

Thanks @sharktide for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link
Copy Markdown

miss-islington-app Bot commented Jun 1, 2025

Thanks @sharktide for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 1, 2025
@sharktide @picnixz @miss-islington
pythongh-133503: clarify compileall -s/-p docs (pythonGH-134756)
e6c6f9e 
(cherry picked from commit fe6f8a3)

Co-authored-by: Rihaan Meher <meherrihaan@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented Jun 1, 2025

GH-134996 is a backport of this pull request to the 3.13 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 1, 2025
@sharktide @picnixz @miss-islington
pythongh-133503: clarify compileall -s/-p docs (pythonGH-134756)
44fb854 
(cherry picked from commit fe6f8a3)

Co-authored-by: Rihaan Meher <meherrihaan@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 1, 2025
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented Jun 1, 2025

GH-134997 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jun 1, 2025
picnixz added a commit that referenced this pull request Jun 1, 2025
@miss-islington @sharktide @picnixz
[3.14] gh-133503: clarify compileall -s/-p docs (GH-134756) (#134997)
5d07d16 
gh-133503: clarify `compileall -s/-p` docs (GH-134756)
(cherry picked from commit fe6f8a3)

Co-authored-by: Rihaan Meher <meherrihaan@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
picnixz added a commit that referenced this pull request Jun 1, 2025
@miss-islington @sharktide @picnixz
[3.13] gh-133503: clarify compileall -s/-p docs (GH-134756) (#134996)
0446628 
gh-133503: clarify `compileall -s/-p` docs (GH-134756)
(cherry picked from commit fe6f8a3)

Co-authored-by: Rihaan Meher <meherrihaan@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@sharktide @picnixz @Pranjal095
pythongh-133503: clarify compileall -s/-p docs (python#134756)
867c5f6 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@sharktide @picnixz @taegyunkim
pythongh-133503: clarify compileall -s/-p docs (python#134756)
dae9e85 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@sharktide @picnixz @Agent-Hellboy
pythongh-133503: clarify compileall -s/-p docs (python#134756)
b8aeb07 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz approved these changes

+1 more reviewer

@mgorny mgorny mgorny approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs Documentation in the Doc dir skip news

Projects

Docs PRs
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

compileall: -s surprisingly makes path relative

3 participants

@sharktide @picnixz @mgorny