Skip to content

Conversation

@GiorgosAlexakis
Copy link
Contributor

This PR adds the ability for someone to use $BRANCH in the branch_name_template which would correspond to the current branch name.

@GiorgosAlexakis
Copy link
Contributor Author

I realized my formatter created unnecessary changes. Updating PR.

@GiorgosAlexakis GiorgosAlexakis force-pushed the use_local_branch_in_branch_name_template branch from eac3959 to e23564e Compare October 10, 2024 09:10
@GiorgosAlexakis
Copy link
Contributor Author

@ZolotukhinM

Copy link
Collaborator

@ZolotukhinM ZolotukhinM left a comment

Choose a reason for hiding this comment

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

Thanks, that's a nice addition! I have one comment, but otherwise this looks good!

@cache
def get_branch_name_base(branch_name_template: str):
username = get_gh_username()
current_branch_name = get_current_branch_name()
Copy link
Collaborator

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

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

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.

@GiorgosAlexakis GiorgosAlexakis force-pushed the use_local_branch_in_branch_name_template branch from e23564e to 5bcd6de Compare October 15, 2024 09:02
@GiorgosAlexakis GiorgosAlexakis force-pushed the use_local_branch_in_branch_name_template branch from 5bcd6de to 3acd32e Compare October 15, 2024 09:12
@ZolotukhinM ZolotukhinM merged commit a3b909e into modular:main Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants