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

internal: refactor package filter #343

Merged
merged 3 commits into from
Sep 25, 2024
Merged

internal: refactor package filter #343

merged 3 commits into from
Sep 25, 2024

Conversation

lijunchen
Copy link
Contributor

Type of Pull Request

  • Bug fix
  • New feature
    • I have already discussed this feature with the maintainers.
  • Refactor
  • Performance optimization
  • Documentation
  • Other (please describe):

Comment on lines 421 to 427
if moonbuild_opt.run_mode == crate::common::RunMode::Test {
if let Some(crate::common::TestOpt {
filter_package: Some(ref filter_package),
..
}) = moonbuild_opt.test_opt
{
let pkgs = packages
.iter()
.filter(|(k, _)| filter_package.contains(Path::new(k)))
.map(|(_, v)| v);
let package_filter = moonbuild_opt.get_package_filter();
if let Some(filter) = package_filter {
let pkgs = packages.iter().filter(|(_, p)| filter(p)).map(|(_, v)| v);
let mut pkg_and_its_deps = HashSet::new();
for pkg in pkgs {
pkg_and_its_deps.extend(get_pkg_and_its_deps(pkg, &packages));
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we could remove the filter here? to get the real all packages

Copy link

Based on the provided git diff output, here are the key observations and suggestions for improvement:

  1. Consistent Use of Package Access Methods:

    • There is a recurring pattern where methods like get_all_packages(), contains_package(), get_package_by_name(), get_filtered_packages(), and others are being used instead of directly accessing the packages field. This suggests a refactoring where the direct access to packages is deprecated in favor of more controlled and potentially more feature-rich methods.
    • Suggestion: Ensure that all parts of the codebase adhere to this new pattern to maintain consistency and leverage the added functionalities provided by these methods.
  2. Filtering Logic for Packages:

    • The filtering logic for packages during tests (filter_package) is being refactored to use a more dynamic approach with a filter function rather than a direct check against a static list.
    • Suggestion: This change enhances flexibility and readability. Ensure that all relevant parts of the codebase are updated to use the new get_package_filter() method consistently.
  3. Optimization and Readability:

    • The refactoring includes changes to reduce code duplication, especially in the generation of build options and lists. This improves maintainability and readability.
    • Suggestion: Continuously review the codebase for opportunities to further reduce duplication and improve readability. Consider using helper functions or macros to encapsulate common patterns.

Overall, these changes indicate a move towards a more encapsulated and controlled access pattern to the packages field, which should improve the maintainability and flexibility of the codebase. Ensuring consistency in adhering to these new patterns across the codebase will be key to realizing these benefits.

@lijunchen lijunchen merged commit 0e4620b into main Sep 25, 2024
8 of 9 checks passed
@lijunchen lijunchen deleted the refactor-package-filter branch September 25, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants