fix: deduplicate submodule status check logic (foundry-rs#13010) #317
fix: deduplicate submodule status check logic (foundry-rs#13010) #317Dargon789 merged 8 commits intoDargon789:masterfrom
Conversation
* update deny for CI * Update more
Co-authored-by: tefyosL-sol <[email protected]>
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
@maximevtush is attempting to deploy a commit to the Foundry development Team on Vercel. A member of the Team first needs to authorize it. |
Reviewer's GuideRefactors and cleans up various utilities across the codebase while deduplicating the git submodule status logic and improving Solar source selection for lint/build flows. Also updates security advisories configuration. Sequence diagram for deduplicated git submodule status checksequenceDiagram
actor Developer
participant ForgeCLI
participant GitUtils
participant GitCommand as GitCommandRunner
Developer->>ForgeCLI: run command requiring submodules_uninitialized
ForgeCLI->>GitUtils: submodules_uninitialized()
GitUtils->>GitUtils: has_missing_dependencies(empty_iterator)
GitUtils->>GitCommand: cmd().args(["submodule", "status"])
GitCommand-->>GitUtils: stdout
GitUtils->>GitUtils: parse lines, check for '-' prefix
GitUtils-->>ForgeCLI: bool (uninitialized_submodules)
ForgeCLI-->>Developer: report based on uninitialized_submodules
Sequence diagram for Solar source selection with ignored paths in lint/buildsequenceDiagram
actor User
participant ForgeCLI
participant LintArgs
participant BuildArgs
participant ProjectCompiler
participant BuildUtils as BuildUtils_get_solar_sources
participant Solar
User->>ForgeCLI: forge lint
ForgeCLI->>LintArgs: execute()
LintArgs->>ProjectCompiler: compile(input_files)
ProjectCompiler-->>LintArgs: ProjectCompileOutput
LintArgs->>BuildUtils: get_solar_sources_from_compile_output(config, output, target_paths, ignored_paths)
BuildUtils->>BuildUtils: collect target source_paths
BuildUtils->>BuildUtils: traverse imports, skip is_ignored(path)
BuildUtils-->>LintArgs: SolcVersionedInput (filtered sources)
LintArgs->>Solar: run lint with SolcVersionedInput
Solar-->>User: lint results
User->>ForgeCLI: forge build
ForgeCLI->>BuildArgs: execute()
BuildArgs->>ProjectCompiler: compile(input_files)
ProjectCompiler-->>BuildArgs: ProjectCompileOutput
BuildArgs->>BuildUtils: get_solar_sources_from_compile_output(config, output, target_paths, ignored_paths=None)
BuildUtils-->>BuildArgs: SolcVersionedInput
BuildArgs->>Solar: optional Solar checks
Solar-->>User: build diagnostics
Class diagram for updated Solar source selection utilitiesclassDiagram
class BuildUtils {
+get_solar_sources_from_compile_output(config: Config, output: ProjectCompileOutput, target_paths: Option~PathBuf[]~, ignored_paths: Option~PathBuf[]~) Result~SolcVersionedInput~
+configure_pcx_from_compile_output(pcx: PcxConfig, config: Config, output: ProjectCompileOutput, target_paths: Option~PathBuf[]~) Result~()~
}
class LintArgs {
+execute() Result~()~
-ignored: Vec~PathBuf~
}
class BuildArgs {
+execute() Result~()~
}
class ProjectCompiler {
+new() ProjectCompiler
+files(files: Vec~PathBuf~) ProjectCompiler
+compile(project: Project) ProjectCompileOutput
}
class ProjectCompileOutput {
+graph() DependencyGraph
}
class DependencyGraph {
+imports(path: Path) Path[]
}
class SolcVersionedInput {
+input: SolcInput
}
class SolcInput {
+sources: HashMap~PathBuf, SourceUnit~
}
LintArgs --> BuildUtils : uses get_solar_sources_from_compile_output
BuildArgs --> BuildUtils : uses get_solar_sources_from_compile_output
BuildUtils --> ProjectCompileOutput : reads imports via graph()
ProjectCompileOutput --> DependencyGraph : returns
SolcVersionedInput --> SolcInput : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @Dargon789, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on refining the codebase through significant cleanup and refactoring efforts. It removes a substantial amount of unused and redundant code across various modules, improving maintainability and reducing the project's footprint. A notable functional enhancement is the introduction of an Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
get_solar_sources_from_compile_output, theis_ignoredcheck uses exactPathequality; ifignored_pathsare specified in a different form thanoutput.graph().imports()(e.g., relative vs absolute, or directory-level ignores), this may not behave as intended—consider normalizing paths and/or supporting prefix/directory-based matching.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_solar_sources_from_compile_output`, the `is_ignored` check uses exact `Path` equality; if `ignored_paths` are specified in a different form than `output.graph().imports()` (e.g., relative vs absolute, or directory-level ignores), this may not behave as intended—consider normalizing paths and/or supporting prefix/directory-based matching.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request is a nice cleanup and refactoring effort. It successfully deduplicates the submodule status check logic by reusing the has_missing_dependencies function, which is the main goal of this PR. A significant amount of unused code has been removed across various crates, which improves the overall maintainability of the codebase. Additionally, it introduces a useful feature to get_solar_sources_from_compile_output to accept ignored paths, which is correctly utilized in the linting command. The dependency advisory list in deny.toml has also been updated. I have one minor suggestion for improving code conciseness.
Motivation
Solution
PR Checklist
68844bf
Summary by Sourcery
Deduplicate git submodule status checking and streamline related utilities while tightening Solar source selection and updating dependency advisories.
Enhancements:
git submodule statusparsing logic.Build:
cargo-denyadvisory ignore list to reflect current RUSTSEC advisories and unused dependencies.