Issue 883: Use Literal type for strength parameter in java_class_decomposition#884
Issue 883: Use Literal type for strength parameter in java_class_decomposition#884MMenshikh wants to merge 3 commits intocqfn:masterfrom
strength parameter in java_class_decomposition#884Conversation
WalkthroughThe updates modify concurrency result handling in the Changes
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
aibolit/__main__.py(1 hunks)aibolit/ast_framework/java_class_decomposition.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: latexmk
- GitHub Check: make
🔇 Additional comments (2)
aibolit/ast_framework/java_class_decomposition.py (2)
4-4: LGTM: Clean import addition for enhanced typing.The addition of
Literalimport enables more precise type annotations and follows Python typing best practices.
46-46: Excellent type safety improvement!Changing the
strengthparameter from genericstrtoLiteral['strong', 'weak']provides several benefits:
- Enhanced compile-time type checking
- Better IDE autocompletion and validation
- Clearer API contract for callers
- Prevents invalid parameter values at development time
The existing runtime validation (lines 62-69) ensures backward compatibility while this change adds proactive type safety.
| results = [] | ||
| with concurrent.futures.ThreadPoolExecutor(max_workers=multiprocessing.cpu_count()) as executor: | ||
| future_results = [executor.submit(run_recommend_for_file, file, args) for file in files] | ||
| concurrent.futures.wait(future_results) | ||
| for future in future_results: | ||
| yield future.result() | ||
| for future in concurrent.futures.as_completed(future_results): | ||
| try: | ||
| results.append(future.result()) | ||
| except Exception as e: | ||
| print(f'Error analyzing file: {e}') | ||
| return results |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improved concurrency with better error handling.
The changes enhance the robustness of concurrent file processing:
- Better error isolation: Individual file analysis failures no longer stop the entire process
- Explicit result collection: Clear accumulation of results using
as_completed() - Enhanced debugging: Error messages provide context about which file failed
Note that this changes the return type from generator to list. Verify that all callers (like line 777) handle this correctly.
For large datasets, consider the memory implications of collecting all results vs the previous streaming approach. Run this script to verify the function usage pattern:
🏁 Script executed:
#!/bin/bash
# Description: Verify how run_thread function results are consumed
# Expected: Should find list() conversion or similar at call sites
rg -A 3 -B 3 "run_thread\(" --type pyLength of output: 729
Update call site for the new run_thread return type
Since run_thread now returns a List instead of a generator, remove the redundant list() wrapper at its call site:
- In aibolit/main.py (around line 884):
- results = list(run_thread(files, args)) + results = run_thread(files, args)
This avoids an unnecessary copy. You may still review memory usage when processing very large file sets, but the change keeps the caller in sync with the new return type.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In aibolit/__main__.py around lines 881 to 889, the call site for run_thread
currently wraps its result in list(), which is redundant since run_thread now
returns a List instead of a generator. Remove the list() wrapper around the call
to run_thread to avoid unnecessary copying and keep the caller consistent with
the updated return type.
| results.append(future.result()) | ||
| except Exception as e: | ||
| print(f'Error analyzing file: {e}') | ||
| return results |
There was a problem hiding this comment.
@MMenshikh I guess you based this PR on top of the branch for PR #881
Could you branch this out of the master?
| def decompose_java_class( | ||
| class_ast: AST, | ||
| strength: str, | ||
| strength: Literal['strong', 'weak'], |
There was a problem hiding this comment.
@MMenshikh while this improves the code from the typing standpoint, it does not help prevent invalid values at runtime. You mentioned that it does help, though, in the PR description. Can you fix the PR description?
2 participants
Replaced
strwithLiteral['strong', 'weak']for thestrengthparameter indecompose_java_class(file:java_class_decomposition.py).This improves type safety and makes the API more self-documenting.
Note: It does not prevent invalid values at runtime, but helps catch issues earlier with static analysis tools and IDEs.
Summary by CodeRabbit
Bug Fixes
Refactor