-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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(rust): Support workspace.members parsing for Cargo.toml analysis #5285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @anfedotoff
Thanks for your work!
I left some comments. Take a look, when you have time, please.
Regards, Dmitriy
@DmitriyLewen, sorry for too long, but I'm done with fixes. Please, have a look, if you have time:). |
Good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anfedotoff I left a few small comments.
I also forgot to ask you to add a test for members.
and one more thing - can you show how Cargo builds a dependency tree for participants.
I'm worried about direct/indirect dependencies.
@anfedotoff Can you take a look on test errors (windows tests are broken)? |
Hmm, honestly, I don't understand why this is happened. Maybe that test is flaky? Could you restart it, please? |
I don't think it's flaky. It shows a file open error. |
This PR is stale because it has been labeled with inactivity. |
@knqyf263 I fixed Windows tests and did some small refactoring for this PR. |
ID: "regex@1.10.2", | ||
Name: "regex", | ||
Version: "1.10.2", | ||
Indirect: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be false (meaning a direct dependency)? It is defined in Cargo.toml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
log.Logger.Warnf("Unable to parse %q: %s", memberPath, err) | ||
continue | ||
} | ||
maps.Copy(dependencies, memberDeps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, workspace.dependencies
is overridden by mistake here and then loses the version constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right.
Thanks!
Fixed in 7b31975
Description
Related issues
Checklist