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(core): dedupe project files in rust #17618

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

FrozenPandaz
Copy link
Collaborator

Current Behavior

When getting config files via rust, config files were not deduped.

Expected Behavior

Getting config files via rust, config files are deduped properly.

Related Issue(s)

Fixes #

@FrozenPandaz FrozenPandaz requested review from a team as code owners June 15, 2023 15:51
@FrozenPandaz FrozenPandaz requested a review from vsavkin June 15, 2023 15:51
@vercel
Copy link

vercel bot commented Jun 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 15, 2023 6:17pm

}
(projects, file_hashes)
});
Ok((projects, file_data))
Ok((
projects.into_iter().map(|(_, config)| config).collect(),
Copy link
Member

Choose a reason for hiding this comment

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

There's a clippy suggestion for this:

Suggested change
projects.into_iter().map(|(_, config)| config).collect(),
projects.into_values().collect(),

globs: &GlobSet,
) {
if globs.is_match(&path) {
let parent = path.parent().unwrap_or(Path::new("")).to_path_buf();
Copy link
Member

Choose a reason for hiding this comment

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

use unwrap_or_else so that it's lazily evaluated.

Suggested change
let parent = path.parent().unwrap_or(Path::new("")).to_path_buf();
let parent = path.parent().unwrap_or_else(|| Path::new("")).to_path_buf();

Comment on lines 39 to 40
if !existing_project
|| (existing_project
Copy link
Member

Choose a reason for hiding this comment

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

This was a clippy warning too. We can remove the second existing_project check, because we already know it exists in this branch

Comment on lines 49 to 56
} else {
if !config_paths.contains_key(&parent) {
config_paths.insert(parent, (path, content));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

clippy:

Suggested change
} else {
if !config_paths.contains_key(&parent) {
config_paths.insert(parent, (path, content));
}
}
} else {
config_paths.entry(parent).or_insert((path, content));
}

Copy link
Member

@Cammisuli Cammisuli left a comment

Choose a reason for hiding this comment

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

There's a bunch of clippy suggestions that should be fixed

@FrozenPandaz FrozenPandaz merged commit 00d07b1 into nrwl:master Jun 15, 2023
@FrozenPandaz FrozenPandaz deleted the dedupe-project-files-in-rust branch June 15, 2023 20:36
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants