polarion_approve_automate() should compare commit hashes to get the git diff.#153
polarion_approve_automate() should compare commit hashes to get the git diff.#153
Conversation
WalkthroughThis pull request updates several functions to support commit-based operations rather than branch-based ones. The changes include modifying function signatures in both the Polarion automated and utility modules to accept new parameters for previous and current commits, and adjusting the command-line interface accordingly. Additionally, the git diff functions have been updated to handle either branch or commit inputs, and a new test function has been added to check for missing required parameters. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/polarion/polarion_utils.py (1)
27-29: Minor type annotation inconsistency.The return type annotation has unnecessary parentheses:
(dict)[str, list[str]]. It should bedict[str, list[str]].-) -> (dict)[str, list[str]]: +) -> dict[str, list[str]]:apps/polarion/polarion_set_automated.py (1)
72-77: Inconsistent parameter passing.This function call doesn't explicitly set
branch=None, unlike the call inremove_approved_tests. While functionally equivalent, this inconsistency could affect maintainability.For consistency with the call on lines 81-86, consider either:
- Explicitly setting
branch=Nonehere:if added_ids := find_polarion_ids( polarion_project_id=polarion_project_id, string_to_match="added", + branch=None, previous_commit=previous_commit, current_commit=current_commit, ):
- Or removing the explicit
branch=Nonefrom the call on lines 81-86 if it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/polarion/polarion_set_automated.py(3 hunks)apps/polarion/polarion_utils.py(2 hunks)tests/polarion/test_polarion_automated.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (4)
tests/polarion/test_polarion_automated.py (1)
17-17: Assertion updated to match new CLI behavior.The assertion was correctly updated to check for "Missing option" instead of the specific project ID error message, aligning with the CLI changes where both
--previous-commitand--current-commitare now required parameters.apps/polarion/polarion_utils.py (1)
67-73: Enhance flexibility with commit-based operations.The function signature has been updated to support commit-based operations, which is consistent with the changes in other functions and aligns with the PR objective.
apps/polarion/polarion_set_automated.py (2)
22-28: Function updated to support commit-based operations.The
remove_approved_testsfunction has been updated to accept both branch-based and commit-based parameters, providing flexibility in how Polarion IDs are retrieved.
81-86: Function call updated to use commit parameters.The call to
remove_approved_testshas been properly updated to pass the commit parameters.
f7e8808 to
9bddaa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/polarion/polarion_set_automated.py (1)
55-56:⚠️ Potential issueCLI option short form conflict: Both options use the same short form
-p.The
--project-id(line 55) and--previous-commit(line 56) both use the same short option-p, which will cause a conflict. Only one of them will be accessible via the short option.Change one of the short options:
@click.option("--project-id", "-p", help="Provide the polarion project id") -@click.option("--previous-commit", "-p", help="Provide previous-commit to compare against") +@click.option("--previous-commit", "-r", help="Provide previous-commit to compare against")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/polarion/polarion_set_automated.py(2 hunks)apps/polarion/polarion_utils.py(2 hunks)tests/polarion/test_polarion_automated.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/polarion/test_polarion_automated.py
- apps/polarion/polarion_utils.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (4)
apps/polarion/polarion_set_automated.py (4)
59-61: Function signature changed to use commits instead of branch.The function no longer accepts a
branchparameter, which is consistent with the CLI changes but is a breaking change for any code directly calling this function.Ensure all callers of this function are updated to use commits instead of branch.
98-99:⚠️ Potential issueUpdate the direct call to match the new function signature.
The function call needs to be updated to provide the required parameters according to the new signature.
This direct call will fail since it doesn't provide the required parameters. Consider using Click's standalone mode to make testing easier:
if __name__ == "__main__": - polarion_approve_automate() + polarion_approve_automate(standalone_mode=False)Or provide default values in a way that works with Click:
if __name__ == "__main__": # Add sys.argv if not present to avoid Click errors if len(sys.argv) <= 1: sys.argv.extend(["--previous-commit", "HEAD~1", "--current-commit", "HEAD"]) polarion_approve_automate()
21-28: LGTM: Good backward compatibility for the function signature.The updated signature for
remove_approved_testsmaintains backward compatibility by making all parameters optional with sensible defaults, which is a good practice when evolving APIs.
67-68: Good error handling for missing parameters.The added check for required parameters with a clear error message is a good defensive programming practice.
9bddaa2 to
27e218a
Compare
|
|
|
/verified |
|
/verified |
polarion_approve_automate() is supposed to run on PR submission. The payload generated contains clear information on before and after commit. It is best to compare those. On submission may repos has configuration to delete the branch immediately. We can avoid that issue as well.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests