Skip to content

resolve $BRANCH as current branch name in branch_name_template #40

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.

Sign up for GitHub

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

Merged
ZolotukhinM merged 1 commit into from
Oct 17, 2024
Merged

resolve $BRANCH as current branch name in branch_name_template #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,4 @@ venv.bak/
pixi.lock

.stack-pr.cfg
.vscode
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,5 +258,5 @@ keep_body=False
remote=origin
target=main
reviewer=GithubHandle1,GithubHandle2
branch_name_template=$USERNAME/stack
branch_name_template=$USERNAME/$BRANCH
```
5 changes: 4 additions & 1 deletion src/stack_pr/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import re
from functools import cache
from subprocess import SubprocessError
from typing import List, NamedTuple, Optional, Pattern

from stack_pr.git import (
branch_exists,
Expand All @@ -67,7 +68,6 @@
run_shell_command,
set_show_commands,
)
from typing import List, NamedTuple, Optional, Pattern

# A bunch of regexps for parsing commit messages and PR descriptions
RE_RAW_COMMIT_ID = re.compile(r"^(?P<commit>[a-f0-9]+)$", re.MULTILINE)
Expand Down Expand Up @@ -565,7 +565,9 @@ def add_or_update_metadata(
@cache
def get_branch_name_base(branch_name_template: str):
username = get_gh_username()
current_branch_name = get_current_branch_name()
Copy link
Collaborator

@ZolotukhinM ZolotukhinM Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling it here is slightly error-prone given how many manipulations the tool does with branches. E.g. I'm concerned that if we don't call it at the right moment, it would return an internal stack branch name rather than the actual name of the user branch.

To prevent this (and make use of the fact that get_branch_name_base is memoized), let's explicitly call get_branch_name_base here:
https://github.com/modularml/stack-pr/blob/5220b763f50b4feb449b0ad82cd24f6a40f45b88/src/stack_pr/cli.py#L1385

Copy link
Contributor Author

@GiorgosAlexakis GiorgosAlexakis Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR! Let me know if the updated approach is what you are expecting. Feel free to merge it after (I don't have access to do so).

Copy link
Collaborator

@ZolotukhinM ZolotukhinM Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks, I'll merge it for you!
One thing I realized though is this can potentially be a source of subtle problems - e.g. if I create a stack while being on branch aaa, then switch to a branch bbb and create another stack, and then decide to merge the two stacks into one. I won't be surprised if something in the current implementation would break if PRs in the same stack use different branch name patterns - that's probably a rare situation and we can address the issues as they arrive.

branch_name_base = branch_name_template.replace("$USERNAME", username)
branch_name_base = branch_name_template.replace("$BRANCH",current_branch_name)
return branch_name_base


Expand Down Expand Up @@ -1388,6 +1390,7 @@ def main():
check_gh_installed()

current_branch = get_current_branch_name()
get_branch_name_base(common_args.branch_name_template)
try:
if args.command != "view" and not is_repo_clean():
error(ERROR_REPO_DIRTY)
Expand Down