Skip to content

Add label, start using it in crawler#197

Merged
simontoens merged 1 commit intosalesforce:masterfrom
simontoens:custom_target
May 20, 2025
Merged

Add label, start using it in crawler#197
simontoens merged 1 commit intosalesforce:masterfrom
simontoens:custom_target

Conversation

@simontoens
Copy link
Contributor

No description provided.

@anuragoyar
Copy link

Potential Risks and Suggestions:

File: src/crawl/pom.py
Code Snippet: def get_pom_generator(workspace, pom_template, artifact_def, dependency):
Comment: The function signature is being changed to remove the dependency parameter, but there are still references to this parameter in the implementation. This could lead to runtime errors. Ensure all callers of this function have been updated to match the new signature.

File: src/crawl/artifactgenctx.py
Code Snippet: self._generator = crawl.pom.get_pom_generator(workspace, pom_template, artifact_def)
Comment: The call to get_pom_generator has been updated to remove the dependency parameter, but make sure the function implementation properly handles this change.

File: src/crawl/crawler.py
Code Snippet: self._discover_dependencies(artifact_def, dep)
Comment: This method has been significantly refactored to use the new label module instead of directly working with dependency objects. Ensure all edge cases are properly handled in the new implementation.

File: src/crawl/pom.py
Code Snippet: def __init__(self, workspace, artifact_def):
Comment: The constructor for AbstractPomGen has been modified to remove the dependency parameter. Verify that all subclasses have been updated accordingly and that they don't rely on this parameter.

File: misc/extdeps_pomgen.py
Code Snippet: super(ThirdPartyDepsPomGen, self).__init__(workspace, artifact_def, dependency=None, pom_template=pom_template)
Comment: The dependency=None parameter is being removed from the parent class constructor call. Ensure this change is compatible with the updated parent class definition.

Summary:

This PR introduces significant architectural changes to the dependency management system in the Salesforce pomgen tool. The main changes include:

  1. Addition of a new label module that provides abstractions for Bazel labels, replacing direct string manipulation.
  2. Refactoring of the dependency discovery process to use the new Label class.
  3. Removal of the dependency parameter from various constructors and methods.
  4. Simplification of the dependency normalization process.
  5. Addition of new tests for the Label class functionality.

The changes appear to be well-structured and include comprehensive test coverage for the new functionality. The PR primarily focuses on internal refactoring to improve code maintainability and readability, rather than changing external behavior.

The most significant risk is in the transition from string-based label handling to the new Label class, but the extensive test coverage should mitigate this risk. The changes to method signatures (particularly removing the dependency parameter) have been consistently applied across the codebase.

This refactoring appears to be a positive improvement to the codebase, making it more maintainable and less error-prone by providing proper abstractions for Bazel labels. The PR includes appropriate tests and seems ready for integration.

@simontoens simontoens merged commit 082bd87 into salesforce:master May 20, 2025
2 checks passed
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.

3 participants