Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs: clarify how multiple values work for various options #2070

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

akx
Copy link
Contributor

@akx akx commented Nov 4, 2024

I spent a long while wondering why comma-separated values wouldn't work for CIBW_SKIP, since this information was cleverly hidden inside the Examples box:

Screenshot 2024-11-04 at 17 35 45

This PR unifies that for all of the multiple-values-supported options; this was already documented in the prose for the environment variable options, so the extra repetition of the same information is now gone from the example box's footer.

I spent a long while wondering why comma-separated values wouldn't work for `CIBW_SKIP`.
@joerick
Copy link
Contributor

joerick commented Nov 5, 2024

I believe we put them in the tabbed box because that rule only applies to environment variables. In TOML, the options are supplied as a list of strings.

@henryiii
Copy link
Contributor

henryiii commented Nov 6, 2024

Maybe we could improve the error message?

@akx
Copy link
Contributor Author

akx commented Nov 6, 2024

I believe we put them in the tabbed box because that rule only applies to environment variables.

I could edit this to mention that in the prose.

Maybe we could improve the error message?

There is no error message – in the case of a malformed SKIP, cibuildwheel just tries to build everything since a pattern like pp*,foo matches nothing (and in the case of my project, failed on PyPy because I use CPython-specific APIs).

It would maybe better to "Do What I Mean", i.e. allow e.g. a [,\s]+ regexp as a separator?

@henryiii
Copy link
Contributor

henryiii commented Nov 6, 2024

cp31{2,3} is a valid expression.

@henryiii
Copy link
Contributor

Let's hold off until we move the default to TOML. That might help shape this.

@henryiii henryiii added the Hold for future release This PR might be complete, but is scheduled to be merged in a future release. Don't merge yet. label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hold for future release This PR might be complete, but is scheduled to be merged in a future release. Don't merge yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants