Don't repeat dependency names in PR title#5915
Conversation
| end | ||
|
|
||
| context "with two dependencies with the same name" do | ||
| let(:dependencies) { [dependency, dependency] } |
There was a problem hiding this comment.
Maybe we should test this with 2 different dependencies (dependency and dependency2) with the same name and different versions?
That way we can be sure the versions are listed correctly
There was a problem hiding this comment.
Are you thinking we should add a test for the PR body? This is only checking the title where we won't show the versions if theres more than one dependency before deduping.
I do like the idea of using different versions in this test case! That way we'll test that it's deduping just by name instead of the entire dependency.
There was a problem hiding this comment.
I improved the test case to have different version of the same named dependency in 3ee451b
Nishnha
left a comment
Nishnha
left a comment
There was a problem hiding this comment.
Generally this looks good to me. I left one comment on a test case but the code change itself looks fine.
| if names.count == 1 | ||
| "requirements for #{names.first}" | ||
| else | ||
| "requirements for #{names[0..-2].join(', ')} and #{names[-1]}" | ||
| end |
There was a problem hiding this comment.
💡 Since we have ActiveSupport available, we could use #to_sentence for this case (docs)
There was a problem hiding this comment.
I had this change stashed so I just pushed it up here in case you want to use any of it: ce8eb68
There was a problem hiding this comment.
Thanks for the suggestions! #to_sentence looks ideal but because it requires adding some new active_support imports I think it'd be better to make that change as a separate PR focused on adding active_support to simplify this class.
| if names.count == 1 | ||
| names.first | ||
| else | ||
| "#{names[0..-2].join(', ')} and #{names[-1]}" | ||
| end |
There was a problem hiding this comment.
Same thought here re: #to_sentence 🙂
We expect this to happen when there are multiple versions of the same dependency.
mctofu
force-pushed
the
mctofu/dedupe-dep-names
branch
from
3ee451b to
126dd42
Compare
|
This is what the example PR looks like now: We could do some additional de-duping at the start of the description but I'll leave that for a future PR. |
4 participants
This improves the PR title to only mention a dependency once if it's being updated to multiple versions.
ex: #5880
Fix #5888