Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

deserialize [package.metadata] and [workspace.metadata] tables #798

Merged
merged 4 commits into from
Jun 17, 2024

Conversation

suaviloquence
Copy link
Contributor

Uses the cargo_toml metadata tables to enable getting workspace and package metadata.

Current specification:

[package.metadata.cargo-semver-checks] 
one = "major"  # override lint "one"'s required semver update to `major`, keeping the lint level default
two = "deny"  # override "two"'s lint level to `deny`, leaving the required semver update default
three = { lint-level = "warn" }  # more verbose syntax to override lint level while leaving update default
four = { required-update = "major" }  # more verbose syntax to override required version update
five = { required-update = "minor", lint-level = "allow" }  # override both required semver update and lint level

[workspace.metadata.cargo-semver-checks]
six = "major" # same syntax as above but workspace-level overrides

Considerations:

  • When we start reading these as config, we will read the configuration from the current-version crate (i.e., --manifest-path). This makes sense e.g. for running cargo semver-checks before a release, might complicate CI pipelines where you can't edit the manifest file? I'm not sure.
  • for the Deserialize implementations for RequiredSemverUpdate and LintLevel enums, I added the #[serde(alias = "$name as kebab-case")] instead of doing #[serde(rename_all = "kebab-case")]. This is because we are also deserializing them in the query .ron files, where we use rust enum PascalCasing to specify the enum variant. This means that e.g., lint = { lint-level = "Deny" } would be valid syntax.
  • To think about: do we want to specify lint names in the metadata table as Cargo.toml kebab-case or as they currently are in snake_case?

For reviewing: I tried to make a smaller PR - most of the length comes from an added test, starting from line 118 in manifest.rs

Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looks good modulo the one typo in a doc comment. I'm quite confident it's a typo, so I'll apply the fix and merge this for the sake of moving faster and saving us both a round-trip. If I got it wrong, let me know and I'll revert.

I think lint names should always be underscored (lower_snake_case), not kebab-case. One day users may enable/disable them via attributes in source code where they will definitely be lower_snake_case, so let's keep things consistent everywhere. I know it's a bit of a deviation from the rest of the file's contents, but it feels like the lesser evil.

src/manifest.rs Outdated Show resolved Hide resolved
@obi1kenobi obi1kenobi enabled auto-merge (squash) June 17, 2024 23:05
@suaviloquence
Copy link
Contributor Author

Sounds good, thanks for the catch!

@obi1kenobi obi1kenobi merged commit fc8040e into obi1kenobi:main Jun 17, 2024
34 checks passed
@obi1kenobi
Copy link
Owner

The considerations section was very, very helpful btw — nicely done!

I'm not worried about CI pipelines having different config than pre-release right now. If it ends up being a problem later, I'm sure we can figure out some CLI flags or other way around it.

Btw in a near-future PR, it would be great to have some end-to-end test cases over realistic workspaces, where the workspace and the package sections aren't in the same Cargo.toml and yet all the right Cargo.toml files are located, parsed, and applied correctly. Ideally, you'd think through various ways that we might have ended up with buggy implementations, and then come up with scenarios where the test would fail if that bug was present: things like "workspace Cargo.toml is read and applied but package's is not" or vice versa, or where the package's Cargo.toml overrides something the workspace set.

The number of possible "gotchas" there is a bit worrying, so it would be lovely to have lots and lots of thoughtful test coverage there as early as possible.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants