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

Check for [workspace] when finding workspace root #30

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

Conversation

mgeisler
Copy link

@mgeisler mgeisler commented Apr 6, 2022

Before, the top-most Cargo.toml file was used. Now we use the nearest Cargo.toml file which contain a line saying [workspace].

This should avoid false positives when Cargo workspaces are nested inside each other: in this case, the inner package will have an empty [workspace] table.

@mgeisler
Copy link
Author

mgeisler commented Apr 6, 2022

Eh, ehm... This fails because the tests interact with each other: the first unit test will read the environment variable and cache it.

So this will need some other approach to be testable (an Integration test could work, but it doesn't have access to the private to_abs_ws_path function).

@bjorn3
Copy link

bjorn3 commented Apr 6, 2022

You could make an integration test which creates a file containing expect![], run it with the option to update the test expectation and then check if it is correctly updated.

Before, the top-most `Cargo.toml` file was used. Now we use the
nearest `Cargo.toml` file which contain a line saying `[workspace]`.

This should avoid false positives when Cargo workspaces are nested
inside each other: in this case, the inner package will have an empty
`[workspace]` table.
@mgeisler mgeisler force-pushed the check-for-workspace branch from 9960b05 to 2d8af43 Compare April 6, 2022 12:17
@mgeisler
Copy link
Author

mgeisler commented Apr 6, 2022

You could make an integration test which creates a file containing expect![], run it with the option to update the test expectation and then check if it is correctly updated.

Ah, that's a good idea! I was shying away from that approach since I feared that I would need to call cargo test or similar... But I see now that I can trigger it all from the integration test. I've updated the commit accordingly.

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