Skip to content

Add max length option to BranchNamer#5338

Merged
jeffwidman merged 1 commit intodependabot:mainfrom
TomNaessens:add-max-length-option
Sep 15, 2022
Merged

Add max length option to BranchNamer#5338
jeffwidman merged 1 commit intodependabot:mainfrom
TomNaessens:add-max-length-option

Conversation

@TomNaessens
Copy link
Copy Markdown

@TomNaessens TomNaessens commented Jul 1, 2022

This PR adds a max length option to the BranchNamer and addresses #3107. Also relevant for the "... and may contain a maximum of 128 characters" part of #396.

This is a backwards compatible change and requires no configuration: when no option is passed, no truncating is performed on the branch name.

As this is my first contribution here, let me know if I missed some conventions, or if there are additional changes expected!

@TomNaessens TomNaessens requested a review from a team as a code owner July 1, 2022 09:33
Copy link
Copy Markdown
Member

@jurre jurre Jul 12, 2022

Choose a reason for hiding this comment

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

I'm worried that we'd end up with duplicate branch names in this scenario, as we'd end up truncating the bit that makes them unique (the version), and I think that could cause issues. Maybe we should truncate the dependency_name_part instead? From the linked issue it seems like this happens when a bunch of dependencies are updated in lockstep, so you end up with a long name part like: rails-and-draper-and-rails-controller-testing-and-rspec-rails-and-font-awesome-rails-and...

Copy link
Copy Markdown

@nudded nudded Jul 12, 2022

Choose a reason for hiding this comment

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

That's a valid concern.

Would you be okay with taking a hash of the full-length branch name and taking the first max_length characters from that if max_length is specified? (At some point we'd have to choose the lesser of 2 evils, we cannot deal with going over the limit, however then you do lose pretty branch names)

edit: your suggestion in the edit is probably better and more in spirit of what you want to achieve though 👌

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah a hash could also work, it's a tad safer I guess (if for some reason the combination of dependency names changes, it's still unique)

Copy link
Copy Markdown
Author

@TomNaessens TomNaessens Sep 2, 2022

Choose a reason for hiding this comment

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

Went for @nudded's suggestion with the safer hash. It's a bit smart in the sense that when part of the branch name can be kept, it is kept. E.g., for the branch "business-and-work-and-desks-and-tables-and-chairs-and-lunch", the results (including default prefixes) are:

80 chars: dependabot/bundler/business-and-work-and-desks-and-tables-and-chairs-and-lunch (fits within 80 chars)
70 chars: dependabot/bundler/business-and2a62321b0f24418298904dedd6dada473213321 (prefixes are saved, but is padded with hash to 70 chars)
50 chars: dependabotd2a62321b0f24418298904dedd6dada473213321 (still shortened including a bit of the prefixes)
40 chars: d2a62321b0f24418298904dedd6dada473213321 (just enough to store the full sha1 hash)
20 chars: d2a62321b0f244182989 (hash is shortened to make it fit within 20 chars)

It doesn't help that much for shorter max lengths, especially if prefixes are involved, but in the case where the max length is set to 150, I hope this will aid users when they can at least see the start of the branch name.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very clean solution @TomNaessens 👍

@jeffwidman
Copy link
Copy Markdown
Member

Approving the CI runs for first-time contributor as the PR seems headed in a generally useful direction.

@TomNaessens
Copy link
Copy Markdown
Author

Sorry, took a while to get the feedback processed. Let me know what you think of the changes!

@SimonSomlai
Copy link
Copy Markdown

@TomNaessens , thanks for opening this, we've also ran into a similar problem. Seems like there are just some linter issues left (most of which can be autocorrected);

Screenshot 2022-09-13 at 10 11 37

@jeffwidman / @jurre, does this need more work, or do the changes look good? :)

@TomNaessens
Copy link
Copy Markdown
Author

@TomNaessens , thanks for opening this, we've also ran into a similar problem. Seems like there are just some linter issues left (most of which can be autocorrected);

Thanks! Should be fixed now 🙇‍♂️

@jeffwidman
Copy link
Copy Markdown
Member

jeffwidman commented Sep 15, 2022

Rebased, once CI passes I'll merge to main. We've been shifting to a "deploy-then-merge" strategy, but for community contributions that's not currently possible due to the way permissions on forks work, so will have to merge first. Probably won't get deployed to GitHub production til tomorrow or Friday, but that's fine in this case since the folks who need this are primarily using GitLab.

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.

5 participants