◐ Shell
reader mode source ↗
Skip to content

bpo-43312: Functions returning default and preferred sysconfig schemes#24644

Merged
pfmoore merged 6 commits into
python:masterfrom
uranusjr:sysconfig-preferred-scheme
Apr 27, 2021
Merged

bpo-43312: Functions returning default and preferred sysconfig schemes#24644
pfmoore merged 6 commits into
python:masterfrom
uranusjr:sysconfig-preferred-scheme

Conversation

@uranusjr

@uranusjr uranusjr commented Feb 25, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Mar 28, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Mar 28, 2021
@uranusjr uranusjr force-pushed the sysconfig-preferred-scheme branch from acadc40 to a0d613f Compare March 29, 2021 20:17
@uranusjr uranusjr marked this pull request as ready for review March 29, 2021 21:36
@uranusjr

uranusjr commented Mar 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Uh, how do I remove the stale label?

@github-actions github-actions Bot removed the stale label Mar 30, 2021
pfmoore
pfmoore approved these changes Apr 6, 2021
View reviewed changes

@pfmoore pfmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I can't say whether the choices in _get_preferred_schemes are correct, but they seem reasonable. Unless there's anything we can check them against, I'd say we should go with them.

hroncok
hroncok reviewed Apr 14, 2021
View reviewed changes

@hroncok hroncok left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

I ❤️ it!

Several non-blocking questions attached.

However, I have a very strong suggestion: Please add tests.

@uranusjr

uranusjr commented Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

I’ve changed get_default_scheme() to use get_preferred_scheme(), and a couple of simple tests for the new functions.

@frenzymadness

frenzymadness commented Apr 15, 2021

Copy link
Copy Markdown
Contributor

The implementation looks good to me.

@uranusjr

uranusjr commented Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

I’m wondering whether we should add documentation for the private _get_preferred_schemes() as well, to tell redistributors this is the function they’re advised to override/modify.

@encukou

encukou commented Apr 16, 2021

Copy link
Copy Markdown
Member

I’m wondering whether we should add documentation for the private _get_preferred_schemes() as well, to tell redistributors this is the function they’re advised to override/modify.

At least a docstring would be great, yes.

@uranusjr

uranusjr commented Apr 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Documentation entry added 👍

Edit: Plus converting %-formatting to f-strings.

@hroncok

hroncok commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

Curiosity question: Should distutils use this?

@uranusjr

uranusjr commented Apr 19, 2021

Copy link
Copy Markdown
Contributor Author

Ideally yes, but distutils’s implementation around this is structured very differently, and tangled with a lot of things inside distutils.command.install, I’m not sure if it’s really worthwhile (or even doable without risking behavioural changes) 😞

zooba
zooba reviewed Apr 19, 2021
View reviewed changes

@zooba zooba left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Would like to see that error message improved, but the rest seems fine to me. I'm just taking on trust that it's sufficient for distros to override what they need, because I don't know that side of it well enough.

@uranusjr

uranusjr commented Apr 27, 2021

Copy link
Copy Markdown
Contributor Author

Another nudge 🙂

@pfmoore

pfmoore commented Apr 27, 2021

Copy link
Copy Markdown
Member

I believe all comments have been addressed, and I see no remaining concerns raised and general approval of the change, so I'm going to merge this based on my approval above.

@pfmoore pfmoore merged commit d925133 into python:master Apr 27, 2021
@uranusjr uranusjr deleted the sysconfig-preferred-scheme branch April 27, 2021 08:46
@carlbordum carlbordum mentioned this pull request May 27, 2022
Closed
@mattip mattip mentioned this pull request Nov 16, 2022
Closed
@FFY00 FFY00 mentioned this pull request Dec 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pfmoore pfmoore pfmoore approved these changes

@zooba zooba zooba left review comments

+2 more reviewers

@gaborbernat gaborbernat gaborbernat left review comments

@hroncok hroncok hroncok left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@uranusjr @frenzymadness @encukou @hroncok @pfmoore @gaborbernat @zooba @the-knights-who-say-ni @bedevere-bot