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

Support glob syntax in workspace members #3979

Merged
merged 6 commits into from May 16, 2017

Conversation

Projects
None yet
6 participants
@hjr3
Contributor

hjr3 commented Apr 29, 2017

Fixes #3911

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Apr 29, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rust-highfive commented Apr 29, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Show outdated Hide outdated src/cargo/core/workspace.rs
}
for path in expanded_list {
let manifest_path = path.join("Cargo.toml");

This comment has been minimized.

@matklad

matklad May 2, 2017

Member

Will this error if there's a path without Cargo.toml matched by the glob? I would say that it's OK to bail in this case, but that just skipping that path would be probably more useful: I often have various assorted folders alongside the workspace packages.

In any case, I think a test case for this would be useful :)

@matklad

matklad May 2, 2017

Member

Will this error if there's a path without Cargo.toml matched by the glob? I would say that it's OK to bail in this case, but that just skipping that path would be probably more useful: I often have various assorted folders alongside the workspace packages.

In any case, I think a test case for this would be useful :)

This comment has been minimized.

@hjr3

hjr3 May 2, 2017

Contributor

Good call out. I will get on it.

@hjr3

hjr3 May 2, 2017

Contributor

Good call out. I will get on it.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 2, 2017

Member

Thanks for the PR @hjr3! Looks good to me but I'd share the same concerns as @matklad, mayb ejust add a test to see what the error looks like?

Member

alexcrichton commented May 2, 2017

Thanks for the PR @hjr3! Looks good to me but I'd share the same concerns as @matklad, mayb ejust add a test to see what the error looks like?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 2, 2017

Member

Also mind updating the workspace documentation to mention that globs may be used?

Member

alexcrichton commented May 2, 2017

Also mind updating the workspace documentation to mention that globs may be used?

@hjr3

This comment has been minimized.

Show comment
Hide comment
@hjr3

hjr3 May 2, 2017

Contributor

@alexcrichton sure, i can update the docs too!

Contributor

hjr3 commented May 2, 2017

@alexcrichton sure, i can update the docs too!

@nipunn1313

This comment has been minimized.

Show comment
Hide comment
@nipunn1313

nipunn1313 May 4, 2017

Contributor

Thanks @hjr3! I'm looking forward to this one (we currently have a big setup with a script to autogenerate the workspace members).

Contributor

nipunn1313 commented May 4, 2017

Thanks @hjr3! I'm looking forward to this one (we currently have a big setup with a script to autogenerate the workspace members).

hjr3 added some commits May 7, 2017

@hjr3

This comment has been minimized.

Show comment
Hide comment
@hjr3

hjr3 May 7, 2017

Contributor

Apologies for taking so long to update this PR. I have updated the docs. Is it ok to reuse the globs URL like I did? It renders correctly with my testing.

I also added the test to shows what happens with a workspace path does not have a Cargo.toml file. As expected, it throws an error. I would expect that someone would add that to the exclude array though. Is that good enough? If it is, does the error message need to be improved? Should I mention that in the docs?

Contributor

hjr3 commented May 7, 2017

Apologies for taking so long to update this PR. I have updated the docs. Is it ok to reuse the globs URL like I did? It renders correctly with my testing.

I also added the test to shows what happens with a workspace path does not have a Cargo.toml file. As expected, it throws an error. I would expect that someone would add that to the exclude array though. Is that good enough? If it is, does the error message need to be improved? Should I mention that in the docs?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 8, 2017

Member

Looks good to me, but the tests seem to be failing? The new invalid_members test also seems to not have an error message :(

Member

alexcrichton commented May 8, 2017

Looks good to me, but the tests seem to be failing? The new invalid_members test also seems to not have an error message :(

fixes
- fix glob missing members test to use proper expectations
- update code to handle case where glob does not match anything
@hjr3

This comment has been minimized.

Show comment
Hide comment
@hjr3

hjr3 May 9, 2017

Contributor

@alexcrichton ok i fixed glob_syntax_invalid_members to have the proper expectations. The invalid_members test was failing because that member path does not actually exist and the glob will ignore it. If expanded the member path returns an empty list, I default to the original (non-expanded) value. That could be weird in some cases though if someone specifies a pattern that does not match anything. It might be better to return an error if the pattern does not match anything?

Contributor

hjr3 commented May 9, 2017

@alexcrichton ok i fixed glob_syntax_invalid_members to have the proper expectations. The invalid_members test was failing because that member path does not actually exist and the glob will ignore it. If expanded the member path returns an empty list, I default to the original (non-expanded) value. That could be weird in some cases though if someone specifies a pattern that does not match anything. It might be better to return an error if the pattern does not match anything?

Show outdated Hide outdated src/cargo/core/workspace.rs
@@ -527,6 +544,18 @@ impl<'cfg> Workspace<'cfg> {
}
}
fn expand_member_path(path: &Path) -> CargoResult<Vec<PathBuf>> {
let path = path.to_str().unwrap();

This comment has been minimized.

@alexcrichton

alexcrichton May 9, 2017

Member

Could the None case be handled here instead of unwrap? (returning an empty Vec)

@alexcrichton

alexcrichton May 9, 2017

Member

Could the None case be handled here instead of unwrap? (returning an empty Vec)

This comment has been minimized.

@hjr3

hjr3 May 10, 2017

Contributor

fixed

@hjr3

hjr3 May 10, 2017

Contributor

fixed

Show outdated Hide outdated src/cargo/core/workspace.rs
@@ -527,6 +544,18 @@ impl<'cfg> Workspace<'cfg> {
}
}
fn expand_member_path(path: &Path) -> CargoResult<Vec<PathBuf>> {
let path = path.to_str().unwrap();
let res = glob(path).map_err(|e| {

This comment has been minimized.

@alexcrichton

alexcrichton May 9, 2017

Member

Could this use .chain_error instead of .map_err?

@alexcrichton

alexcrichton May 9, 2017

Member

Could this use .chain_error instead of .map_err?

This comment has been minimized.

@hjr3

hjr3 May 10, 2017

Contributor

I may be missing something here, but I don't see how .chain_error can be used with Result<glob::Paths, glob::PatternError>.

@hjr3

hjr3 May 10, 2017

Contributor

I may be missing something here, but I don't see how .chain_error can be used with Result<glob::Paths, glob::PatternError>.

Show outdated Hide outdated src/cargo/core/workspace.rs
human(format!("could not parse pattern `{}`: {}", &path, e))
})?;
res.map(|p| {
p.or_else(|e| {

This comment has been minimized.

@alexcrichton

alexcrichton May 9, 2017

Member

As above, could this use chain_error?

@alexcrichton

alexcrichton May 9, 2017

Member

As above, could this use chain_error?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 9, 2017

Member

Nah this seems like a reasonable interpretation to me I think, thanks!

Member

alexcrichton commented May 9, 2017

Nah this seems like a reasonable interpretation to me I think, thanks!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 10, 2017

Member

@hjr3 mind updating to using chain_error instead of map_err in a few locations as well?

Member

alexcrichton commented May 10, 2017

@hjr3 mind updating to using chain_error instead of map_err in a few locations as well?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton
Member

alexcrichton commented May 16, 2017

@bors: r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 16, 2017

Contributor

📌 Commit f00c223 has been approved by alexcrichton

Contributor

bors commented May 16, 2017

📌 Commit f00c223 has been approved by alexcrichton

@hjr3

This comment has been minimized.

Show comment
Hide comment
@hjr3

hjr3 May 16, 2017

Contributor

@alexcrichton thanks, i was just about to push this same patch. it took me some time to figure out how to get the chain_error working for glob.

Contributor

hjr3 commented May 16, 2017

@alexcrichton thanks, i was just about to push this same patch. it took me some time to figure out how to get the chain_error working for glob.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 16, 2017

Contributor

⌛️ Testing commit f00c223 with merge 828a9c5...

Contributor

bors commented May 16, 2017

⌛️ Testing commit f00c223 with merge 828a9c5...

bors added a commit that referenced this pull request May 16, 2017

Auto merge of #3979 - hjr3:issue-3911, r=alexcrichton
Support glob syntax in workspace members

Fixes #3911
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 16, 2017

Member

Oh sorry about that! I was just going through old Cargo PRs and figured this could use a bit of a boost :)

Member

alexcrichton commented May 16, 2017

Oh sorry about that! I was just going through old Cargo PRs and figured this could use a bit of a boost :)

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors May 16, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 828a9c5 to master...

Contributor

bors commented May 16, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 828a9c5 to master...

@bors bors merged commit f00c223 into rust-lang:master May 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment