-
Notifications
You must be signed in to change notification settings - Fork 21
⚡️ Speed up function get_java_formatter_cmd by 217% in PR #1199 (omni-java)
#1621
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
base: omni-java
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -432,13 +432,11 @@ def get_java_formatter_cmd(formatter: str, build_tool: JavaBuildTool) -> list[st | |
| if formatter == "google-java-format": | ||
| return ["google-java-format --replace $file"] | ||
| if formatter == "spotless": | ||
| if build_tool == JavaBuildTool.MAVEN: | ||
| return ["mvn spotless:apply -DspotlessFiles=$file"] | ||
| if build_tool == JavaBuildTool.GRADLE: | ||
| return ["./gradlew spotlessApply"] | ||
| return ["spotless $file"] | ||
| return _SPOTLESS_COMMANDS.get(build_tool, ["spotless $file"]) | ||
| if formatter == "other": | ||
| click.echo("In codeflash.toml, please replace 'your-formatter' with your formatter command.") | ||
| if not hasattr(get_java_formatter_cmd, "_warning_shown"): | ||
| click.echo("In codeflash.toml, please replace 'your-formatter' with your formatter command.") | ||
| get_java_formatter_cmd._warning_shown = True | ||
| return ["your-formatter $file"] | ||
| return ["disabled"] | ||
|
|
||
|
|
@@ -544,3 +542,9 @@ def get_java_test_command(build_tool: JavaBuildTool) -> str: | |
| if build_tool == JavaBuildTool.GRADLE: | ||
| return "./gradlew test" | ||
| return "mvn test" | ||
|
|
||
|
|
||
| _SPOTLESS_COMMANDS = { | ||
| JavaBuildTool.MAVEN: ["mvn spotless:apply -DspotlessFiles=$file"], | ||
| JavaBuildTool.GRADLE: ["./gradlew spotlessApply"], | ||
| } | ||
|
Comment on lines
+547
to
+550
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutable shared state: The dictionary values are mutable lists shared across all callers. If any caller mutates the returned list (e.g., Not a bug with current callers, but a latent risk. Consider using tuples as values and converting to list in the return: _SPOTLESS_COMMANDS = {
JavaBuildTool.MAVEN: ["mvn spotless:apply -DspotlessFiles=$file"],
JavaBuildTool.GRADLE: ["./gradlew spotlessApply"],
}Or return |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint failure (SLF001) & mypy error (attr-defined): The function-attribute pattern
get_java_formatter_cmd._warning_showntriggers ruff SLF001 (private member access) and mypyattr-definedsince functions don't declare this attribute. This will fail CI pre-commit checks.Additionally, this changes user-visible behavior: the original code displayed the warning on every call, while this version only shows it once per process. In the current usage (called once in the CLI setup flow), this is harmless, but it's a semantic change that should be intentional.
Consider using a module-level flag instead:
Where
_OTHER_WARNING_SHOWNand_mark_other_warning_shown()are defined at module scope (usingglobalin the setter). This avoids both the SLF001 and mypy issues.