Skip to content
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.

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

Allow passing a base branch that doesn't have version info #70

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Jackenmen
Copy link
Contributor

@Jackenmen Jackenmen commented Jul 10, 2022

Resolves #69

I could add enforcement of something here if this is too lax but I'm not sure what kind of check that could be. I suppose I could also add an option to .cherry_picker.toml that needs to be explicitly enabled to allow passing non-version branches if that makes it any better.

I'm aware this issue wasn't accepted but I kinda felt like doing it anyway, even if it doesn't end up merged :P

@Mariatta
Copy link
Member

Mariatta commented Oct 4, 2022

I think this change is reasonable.

@Jackenmen
Copy link
Contributor Author

Tests are currently failing on main and so this PR is affected, I fixed them in #76.

@Mariatta Mariatta added the hacktoberfest-accepted Accepted for hacktoberfest label Oct 4, 2022
@Mariatta
Copy link
Member

Mariatta commented Oct 4, 2022

I think allowing it to be configured via the toml file is a good idea as well. Would you be able to add that in a separate PR?

@Jackenmen
Copy link
Contributor Author

I can do it here or in a separate PR, whatever you prefer. Can probably get to it tomorrow.

I assume you would want this to be a true/false setting determining whether branches without version info are allowed (defaulting to false to keep the current behavior intact)?

@Mariatta
Copy link
Member

Mariatta commented Oct 4, 2022

I think it should support the original behavior if not supplied.

@Jackenmen
Copy link
Contributor Author

Jackenmen commented Oct 5, 2022

Alright, I added a require_version_in_branch_name option. Quite a mouthful but I couldn't really come up with anything else that was still clear about what the option is. At most I could maybe manage to make the name 4-5 characters shorter but the names I came up with were, in my opinion, a lot more vague than the current name.


While looking into this, I also realized that I suggested adding such an option in the PR's description which I didn't really realize when you asked me to add it 😄

cherry_picker/cherry_picker.py Outdated Show resolved Hide resolved
cherry_picker/cherry_picker.py Outdated Show resolved Hide resolved
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
Jackenmen added a commit to Cog-Creators/Red-GitHubBot that referenced this pull request Aug 27, 2023
Jackenmen added a commit to Cog-Creators/Red-GitHubBot that referenced this pull request Aug 27, 2023
@Jackenmen
Copy link
Contributor Author

I created python/miss-islington#639 to make miss-islington work with this PR since I realized that this is going to be necessary now that this also adds a new configuration option.

@Jackenmen
Copy link
Contributor Author

I addressed the review and resolved the merge conflicts. Seems like I may have missed the merge window for the release though 😄

If by the time this is looked at by a maintainer new conflicts arise, just let me know and I'll resolve them - maybe I'll finally be able to use upstream directly :)

@webknjaz
Copy link
Contributor

@hugovk mind taking a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted for hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider laxing the requirements for maintenance branch names?
4 participants