Skip to content

Add pyproject.toml manifest support (PEP 621) - #1842

Open
ChrisJr404 wants to merge 4 commits into
o2sh:mainfrom
ChrisJr404:pyproject-manifest
Open

Add pyproject.toml manifest support (PEP 621)#1842
ChrisJr404 wants to merge 4 commits into
o2sh:mainfrom
ChrisJr404:pyproject-manifest

Conversation

@ChrisJr404

Copy link
Copy Markdown

Closes #1590. This adds pyproject.toml to the set of manifests onefetch understands, so Python projects show a dependency count, description, version and license just like Cargo and npm projects already do.

Following the discussion on the issue, I focused on the format specified by the PEPs rather than tool-specific layouts. A new ManifestType::PyProject reads the PEP 621 [project] table:

  • number_of_dependencies from project.dependencies
  • name / version / description from their project.* fields
  • license from project.license, handled as either a PEP 639 SPDX string or the older { text = "..." } table. A { file = "LICENSE" } table carries no identifier, so it's left unset and onefetch falls back to detecting the license from the repo as before.

Tool-specific tables such as [tool.poetry] use a different shape and are intentionally out of scope here.

Parsing uses toml, which was already in the tree via cargo_toml, so Cargo.lock only gains the existing version as a direct dependency of the manifest crate. In the Dependencies field this reads as e.g. 3 (pyproject.toml).

Tests

  • manifest/tests/pyproject.rs with a tests/fixtures/pyproject/ fixture, mirroring the existing cargo/npm integration tests.
  • A unit test in manifest/src/lib.rs covering the three PEP 621 license forms (SPDX string, { text } table, { file } table).
  • Verified end to end against a scratch git repo containing a pyproject.toml: onefetch prints Dependencies: 3 (pyproject.toml) and picks up the description, version and license. cargo fmt --all --check, cargo clippy, and the full test suite pass.

Parse a repository's pyproject.toml [project] table so onefetch reports the
project's dependency count, name, version, description and license for Python
projects, the same way it already does for Cargo.toml and package.json.

@spenserblack spenserblack left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! Mostly looks good, I just have a few nitpicks.

Comment thread manifest/src/lib.rs Outdated
pub enum ManifestType {
Npm,
Cargo,
#[strum(to_string = "pyproject.toml")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we want to set the to_string to "pyproject.toml"? We don't call the Npm variant "package.json", or the Cargo variant "Cargo.toml".

Comment thread manifest/src/lib.rs Outdated
Comment on lines +130 to +135
// A `license = { file = "LICENSE" }` table carries no identifier, so leave it
// unset and let onefetch fall back to detecting the license from the repo.
let license = project.license.and_then(|license| match license {
PyProjectLicense::Spdx(spdx) => Some(spdx),
PyProjectLicense::Table { text } => text,
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was a bit confused by this for a moment, but, if I understand it correctly, the value of license.text (if license is a table) may also be the SPDX identifier? But it seems like license.text may be any string if the project uses a non-standard license? Could you clarify this in the comment?

Comment thread manifest/src/lib.rs Outdated
Comment on lines +162 to +178
// SPDX expression string (PEP 639)
let spdx: PyProjectTable = toml::from_str("license = \"MIT\"").unwrap();
assert!(matches!(spdx.license, Some(PyProjectLicense::Spdx(s)) if s == "MIT"));

// `{ text = "..." }` table (older PEP 621 form)
let text: PyProjectTable = toml::from_str("license = { text = \"Apache-2.0\" }").unwrap();
assert!(
matches!(text.license, Some(PyProjectLicense::Table { text: Some(t) }) if t == "Apache-2.0")
);

// `{ file = "LICENSE" }` table carries no identifier
let file: PyProjectTable = toml::from_str("license = { file = \"LICENSE\" }").unwrap();
assert!(matches!(
file.license,
Some(PyProjectLicense::Table { text: None })
));
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of using 3 assert!, I think you can use rstest to make this 3 test cases instead. That way a prior failure wouldn't block the following test cases from running.

… comment, use rstest

- Remove the strum to_string override so PyProject renders like the other
  variants (Npm/Cargo) instead of pyproject.toml.
- Expand the PyProjectLicense doc/comment to explain that license.text is
  free-form and only conventionally an SPDX identifier.
- Convert the three license-form assertions into rstest cases so one failure
  no longer masks the others.
@ChrisJr404

Copy link
Copy Markdown
Author

Thanks for the review! Pushed a commit addressing all three:

  1. to_string = "pyproject.toml" — good catch, that was inconsistent with Npm/Cargo. Dropped the override so PyProject renders via the default variant name, matching the others.

  2. license.text comment — you've got it exactly right: text is free-form. It's conventionally an SPDX identifier (e.g. MIT), but PEP 621 permits any string, including the full text of a non-standard license. We surface whatever's there verbatim rather than trying to validate it as SPDX. I expanded the doc comment on PyProjectLicense and the inline comment in parse_pyproject_manifest to spell this out, and to note that a { file = ... } table carries no identifier (so we fall back to repo license detection).

  3. assert!rstest — converted to three #[case]s so a failure in one form no longer blocks the others. Added rstest as a dev-dependency on the onefetch-manifest crate (the workspace already pins 0.26.1).

@spenserblack spenserblack left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks! Just these last two changes and LGTM!

Comment thread manifest/Cargo.toml Outdated
Comment thread manifest/src/lib.rs Outdated
@spenserblack

Copy link
Copy Markdown
Collaborator

I hate to do this, but one more thing, since it looks like you've managed to author over 90 pull requests today alone: have you read our contributing guidelines (including our AI policy)?

@spenserblack spenserblack left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oops, I thought just using rstest.workspace = true was enough to synchronize the versions, but looks like a little bit more work is needed.

TBH you can just revert bd4491b. I'll look into utilizing that feature some other time.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add parsing of pyproject.toml

2 participants