BWDO-778 sc branch rename, rm_merged and more#66
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors ticketing support into sc.services.tickets so it can be reused across commands, introduces new branching commands (branch rename, branch rm_merged, show merged_release), and standardizes error handling via a new sc.exceptions + shared sc.prompter.
Changes:
- Extract ticket models/config/service into
src/sc/services/tickets/*and migrate review tooling to use it. - Add new branching commands:
sc branch rename,sc branch rm_merged, andsc show merged_releasewith accompanying tests. - Improve robustness of branch deletion behavior (handle non-git dirs and failed deletions more gracefully) and unify exception types.
Reviewed changes
Copilot reviewed 31 out of 33 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tickets/test_ticket_service.py | New unit tests for extracted TicketService behavior. |
| tests/review/test_ticket_updater.py | Update review tests to use the new ticket service API and Ticket wrapper. |
| tests/review/test_ticket_service.py | Remove old review-scoped ticket service tests after extraction. |
| tests/review/test_git_host_service.py | Update to new ConfigError exception source/type. |
| tests/branching/test_branch_rm_merged.py | New tests for branch rm_merged command behavior. |
| tests/branching/test_branch_rename.py | New integration-ish tests for branch rename behavior. |
| src/sc/services/tickets/ticketing_instances/ticketing_instance.py | Adjust typing imports to avoid runtime circular dependencies. |
| src/sc/services/tickets/ticketing_instances/ticket_instance_factory.py | Update exception import to the new tickets exception module. |
| src/sc/services/tickets/ticketing_instances/instances/redmine_instance.py | Build TicketData + Ticket wrapper; change Redmine field extraction. |
| src/sc/services/tickets/ticketing_instances/instances/jira_instance.py | Build TicketData + Ticket wrapper for Jira. |
| src/sc/services/tickets/ticketing_instances/instances/init.py | New exports for Jira/Redmine instances. |
| src/sc/services/tickets/ticketing_instances/init.py | New exports for ticketing instance abstractions/factory. |
| src/sc/services/tickets/ticket.py | New TicketData + Ticket wrapper to centralize formatting + comment addition. |
| src/sc/services/tickets/ticket_service.py | New shared ticket service (branch parsing, prompting, instance creation). |
| src/sc/services/tickets/ticket_config.py | New ticket host config model + config access layer. |
| src/sc/services/tickets/exceptions.py | Move ticket exceptions onto ScError base. |
| src/sc/services/tickets/init.py | New exports for tickets package. |
| src/sc/review/ticket_updater.py | Switch ticket updater to use TicketService.get_ticket_from_branch() and Ticket.add_comment(). |
| src/sc/review/ticket_service.py | Remove old review-scoped ticket service (replaced by shared service). |
| src/sc/review/review.py | Update exception handling to catch ScError instead of review-scoped base exception. |
| src/sc/review/review_config.py | Remove ticket config from review config; switch to sc.exceptions.ConfigError. |
| src/sc/review/prompter.py | Remove review-scoped prompter (replaced by shared sc.prompter). |
| src/sc/review/git_host_service.py | Switch remote-url failure exception to ConfigError. |
| src/sc/prompter.py | New shared prompter used across features. |
| src/sc/project_cli.py | Add new CLI commands/groups for branching + merged release output. |
| src/sc/exceptions.py | New shared exception hierarchy (ScError, ConfigError, PermissionsError). |
| src/sc/branching/exceptions.py | Make ScInitError derive from ScError. |
| src/sc/branching/commands/show.py | Add ShowMergedRelease command implementation. |
| src/sc/branching/commands/push.py | Push tags during push operation. |
| src/sc/branching/commands/delete.py | Make delete more resilient (invalid repos, deletion failures). |
| src/sc/branching/commands/branch_rm_merged.py | New implementation for removing merged branches. |
| src/sc/branching/commands/branch_rename.py | New implementation for renaming branches locally/remotely. |
| src/sc/branching/branching.py | Wire new commands + unify error handling to ScError. |
Comments suppressed due to low confidence (1)
src/sc/services/tickets/exceptions.py:49
TicketIdentifierNotFoundis defined twice in this module, so the second definition overwrites the first. This makes tracebacks/docs confusing and can hide future changes to the earlier class body/docstring.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
| return | ||
|
|
||
| try: | ||
| remote = self._remote_name(repo, remote_name) |
There was a problem hiding this comment.
Renaming the remote branch is optional based on the use of the --r cli option.
I think we'd be better off making the option --remote, I worry any variation of -r looks like it's recursive, which could be confusing with repo projects.
There was a problem hiding this comment.
In the old one and tbf I just realised this now but --r is used to rename all branches in the repo workspace and without this it just renames for the singular repo. Old version always renames remote. The old behaviour of using --r to rename all in workspace is weird as it's the opposite of other commands which take a flag to just rename the singular repo instead of taking a flag to operate on all of them
| self.new_branch, | ||
| ) | ||
|
|
||
| def _rename_branch( |
There was a problem hiding this comment.
I'd break this into 2 functions. One for the local branch rename and one of the remote branch rename. We flip between local and remote commands here and it's get confusing.
There was a problem hiding this comment.
I split out the remote checking into its own func which should be less confusing hopefully. The reason I didn't have 2 separate functions is it has a check for a remote branch already existing in the name you specified and then it won't let you rename the local.
| def _remote_branch_commit( | ||
| self, | ||
| repo: Repo, | ||
| remote_name: str, | ||
| branch_name: str) -> str | None: | ||
| out = repo.git.ls_remote("--heads", remote_name, f"refs/heads/{branch_name}") | ||
|
|
||
| for line in out.splitlines(): | ||
| commit, ref = line.split() | ||
| if ref == f"refs/heads/{branch_name}": | ||
| return commit |
There was a problem hiding this comment.
Given the usage of this, surely it should just be a _has_remote and a bool return?
There was a problem hiding this comment.
It's also used to check if there exists a remote branch with the new name which is the same commit as the local branch we're renaming. Which is pretty unlikely but allows it to go through in this situation.
| for line in git_log.splitlines(): | ||
| try: | ||
| identifier, ticket_num = ticket_service.get_ref_from_branch(line) | ||
| ref = (identifier, ticket_num) | ||
| if ref in found_refs: | ||
| continue | ||
|
|
||
| ticket = ticket_service.get_ticket(identifier, ticket_num) | ||
| if self.wiki: | ||
| self._print_wiki(ticket, identifier, ticket_num) |
There was a problem hiding this comment.
Doesn't this result in the same ticket being printed multiple times if the user puts the ticket number in the commit message each time they commit on the same branch?
There was a problem hiding this comment.
I use
git_log = repo.git.log(
f"{prev_release}...{curr_release}", format="%s", first_parent=True, merges=True)
Where merges = True only does merge commits. So it gets the auto generated messages from merging feature branches which has the branch name in them. (Merged x branch into develop).
| self.assertFalse(_remote_branch_exists(proj_repo, "feature/donut")) | ||
| self.assertTrue(_remote_branch_exists(proj_repo, "feature/pizza")) | ||
|
|
||
| def test_branch_rename_can_be_rerun(self): |
There was a problem hiding this comment.
Why is this a test we need? If the branch we're trying to rename doesn't exist surely we want it to display an error message and abort?
There was a problem hiding this comment.
I want it to run through as a no op. Part of the reason branch rename is quite ugly is me trying to avoid the problem I fixed in sc delete where the user can experience an error halfway through and be blocked from rectifying it by rerunning the same command. I want the user to be able to fix any particular errored repo and be able to just run the same command again and it will resolve errored repos.
There was a problem hiding this comment.
Maybe the real best way of doing this should be that it evaluates all branch states/conflicts are correct as a beginning step and only then starts renaming, however, this still causes some problems if there any unexpected errors midway through so I do quite like the idea that it tries to do what the user wants.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 34 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/sc/services/tickets/exceptions.py:49
TicketIdentifierNotFoundis defined twice in this module; the second definition overwrites the first and makes the file harder to reason about. Keep a single definition (and adjust its docstring if needed).
| while True: | ||
| identifier = self._prompter.ask("Prefix") | ||
| if identifier in ticket_conf: | ||
| break | ||
| logger.info(f"Prefix {identifier} not found in instances") |
| host_identifiers = self._config.get_identifiers() | ||
| identifiers_pattern = "|".join(map(re.escape, host_identifiers)) | ||
| return re.compile( | ||
| fr'(?:^|/)({identifiers_pattern})[-_]?(\d+)', | ||
| re.IGNORECASE | ||
| ) |
| def get_config(self) -> dict[str, TicketHostModel]: | ||
| """Return all ticketing instance configs keyed by identifier.""" | ||
| return {k: TicketHostModel(**v) for k,v in self._ticket_config.get_config().items()} | ||
|
|
| for branch in filtered_branches: | ||
| ticket = ticket_service.get_ticket_from_branch(branch.name) | ||
| print(ticket.to_terminal(one_line=True)) | ||
| print(branch.name) |
| def run_git_command(self): | ||
| repo = Repo(self.top_dir) | ||
| remote = repo.remotes[0].name | ||
| repo.git.push("-u", remote, self.branch.name) | ||
| repo.git.push("--tags") |
I can only apologise for a bit of a monster review, been quite good at not doing these for a while.
Things doing:
Moved ticket service out into it's own section so it could be reused in sc branch rm_merged and sc show merged_release.
Created sc branch rm_merged, sc branch rename, sc show merged_release.
Fixed an issue in sc delete which would never complete on rerun if it failed the first time around.