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

fix: inject version for path dependencies in workspace virtual manifests #238

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

Conversation

tomkarw
Copy link
Contributor

@tomkarw tomkarw commented Apr 29, 2024

There was an unhandled case in version injection task.

For a project with virtual manifest like this:

# in /Cargo.toml

[workspace.dependencies]
subcrate = { path = "./subcrate" }

Kraken would not inject version and registry attributes.

This MR fixes it, by correctly injecting versions for dependencies declared in workspaces with virtual manifest.

Decisions:

  • Add _raw attribute to most manifest related classes, store raw JSON data there (without removing the handled fields)
  • For fields that already had a subset of this, rename the field to _raw and use a getter with deprecation warning for backward compatibility
  • All to_json methods could be simplified, as they can simply return self._raw

Copy link
Contributor

@NiklasRosenstein NiklasRosenstein left a comment

Choose a reason for hiding this comment

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

LGTM overall; would be nice to have a test that checks if the version gets injected correctly into the workspace dependencies!

@tomkarw tomkarw force-pushed the tk/fix-inject-version-virtual-manifests branch 3 times, most recently from 5a51b19 to 677a91b Compare April 30, 2024 08:45
@tomkarw tomkarw self-assigned this Apr 30, 2024
@tomkarw tomkarw force-pushed the tk/fix-inject-version-virtual-manifests branch 3 times, most recently from 8e3755c to 40c32af Compare April 30, 2024 17:29
@tomkarw tomkarw force-pushed the tk/fix-inject-version-virtual-manifests branch from 40c32af to 5ea2697 Compare April 30, 2024 21:05
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.

2 participants