Allow analysis to work on binary crates with libraries #239
Conversation
src/build.rs
Outdated
| .map(|p| p.name) | ||
| .collect() | ||
| } | ||
| fn crate_names() -> impl Iterator<Item = String> { |
There was a problem hiding this comment.
Making this a free function rather than method is fine, but the other refactoring I think is not worth doing - iiuc, it adds some complexity only for the sake of removing a single allocation, which I think is unlikely to be significant.
There was a problem hiding this comment.
I don't think it really adds complexity, just a "what's happening here?" with the mapping of an empty vec.
We can replace it with:
fn crate_names() -> impl Iterator<Item = String> {
cargo_metadata::metadata_deps(None, true)
.map(|metadata| metadata.packages)
.unwrap_or_default()
.into_iter()
.map(|package| package.name)
}In fact, if it's a one-liner we might as well inline it into clean_analysis
src/build.rs
Outdated
| if !crate_names.contains(&name) { | ||
| info!("deleting {:?}", entry.path()); | ||
| if !crate_names.contains(&match_name) { | ||
| println!("deleting {:?}", entry.path()); |
| &name[3..] | ||
| } else { | ||
| &name | ||
| }; |
There was a problem hiding this comment.
Why change to the more verbose idiom here?
There was a problem hiding this comment.
I'm not clear if moving the "lib" check is significant - what is the effect?
There was a problem hiding this comment.
Because we need &name still to bucket libX and X seperately (unless that's not what you meant)
src/build.rs
Outdated
| @@ -61,10 +61,16 @@ impl Builder { | |||
|
|
|||
| // Remove any old or duplicate json files. | |||
| fn clean_analysis(&self) { | |||
| let crate_names = self.crate_names(); | |||
| let crate_names = crate_names() | |||
| .map(|name| name.replace("-", "_")) | |||
There was a problem hiding this comment.
If you want to avoid duplicating the collect, you could move this map to crate_names to avoid the new complexity there.
There was a problem hiding this comment.
Or could combine with the following map on line 67
There was a problem hiding this comment.
You need to collect twice to have somewhere for the &strs to point to. You can avoid it using a more verbose idiom using iter().any(|s| s == foo) instead of contains but I didn't think it was necessary because this isn't a particularly hot path.
There was a problem hiding this comment.
I think it is worth using any rather than contains for the sake of cleaner code here and in crate_names(), if not for performance.
|
@nrc Because cargo generates a metadata file with a name of the format |
eira-fransham
force-pushed
the
fix-getting-metadata
branch
from
9297971
to
09e3bc2
Compare
|
Thanks for the change in the last commit. Would you also be able to address the comment about the |
|
Thanks for the last change and the PR, and sorry it has lingered so long. |
Closes #235