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

feat: support PEP 723 with a toml load function #811

Merged
merged 6 commits into from
Apr 12, 2024

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Apr 7, 2024

This is the suggestion from #808. As detailed there, I think this is a flexible first step that will help noxfile authors support PEP 723 in scripts, and could have a followup later that provides a more integrated solution.

This also allows noxfile authors to finally integrate with pyproject.toml on all versions of Python, rather than requiring nox be installed on Python 3.11+, by ensuring that a toml library is always present. You might need to read the version, etc. from pyproject.toml in your noxfile, and now you can.

I made all the errors ValueError because that's what the PEP did in the example implementation. Happy to make them something else if there's something better.

@cjolowicz
Copy link
Collaborator

Should we fail the session instead of raising ValueError? Nox can be more opinionated about the error handling than say a library, and it would avoid the traceback.

@henryiii
Copy link
Collaborator Author

henryiii commented Apr 7, 2024

Hmm, we could, but most of the improperly configured calls seem to also raise ValueError, such as

raise ValueError("The environment does not have a bin directory.")

and

nox/nox/sessions.py

Lines 388 to 393 in 956f10c

if not args:
raise ValueError("At least one argument required to run().")
if len(args) == 1 and isinstance(args[0], (list, tuple)):
msg = "First argument to `session.run` is a list. Did you mean to use `session.run(*args)`?"
raise ValueError(msg)

Signed-off-by: Henry Schreiner <[email protected]>
nox/__init__.py Outdated Show resolved Hide resolved
@henryiii
Copy link
Collaborator Author

henryiii commented Apr 8, 2024

Current name (from the issue) is nox.project.load_toml. nox.project is imported by default.

Copy link
Collaborator

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

Looks great!

Do we want to mark this as a provisional API in some way?

It feels like new territory. We may want to adapt to evolving packaging standards and move around or rename things without going through deprecation.

requirements-test.txt Show resolved Hide resolved
@henryiii
Copy link
Collaborator Author

henryiii commented Apr 12, 2024

Do we want to mark this as a provisional API in some way? ...

I think what we've added here is pretty conservative. nox.project is a place for project related tools. load_toml does exactly that, it loads toml and nothing more. (It does support PEP 723, but that's accepted so it isn't allowed to change significantly without a new PEP). We've left it up to the user to handle converting pyproject.toml's and script blocks into useable information.

If we add more things here, then possibly. If we have a session_from_script helper or something, that could be provisional. Or helpers for parametrizing over the Python versions allowed. Etc.

(But happy to call this provisional too if you prefer)

Co-authored-by: Claudio Jolowicz <[email protected]>
requirements-test.txt Outdated Show resolved Hide resolved
@henryiii henryiii merged commit d6e1906 into wntrblm:main Apr 12, 2024
22 checks passed
@henryiii henryiii deleted the henryiii/feat/pep723 branch April 12, 2024 22:10
@henryiii henryiii mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants