Skip to content

Don't repeat dependency names in PR title #5915

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
mctofu merged 3 commits into from
Oct 24, 2022
Merged

Don't repeat dependency names in PR title #5915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
053b38e
Don't repeat dependency names in PR title
mctofu Oct 18, 2022
e5635fa
lint
mctofu Oct 18, 2022
126dd42
Better example for same name test
mctofu Oct 19, 2022
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
18 changes: 14 additions & 4 deletions common/lib/dependabot/pull_request_creator/message_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,16 @@ def library_pr_name
"#{from_version_msg(old_library_requirement(dependencies.first))}" \
"to #{new_library_requirement(dependencies.first)}"
else
names = dependencies.map(&:name)
"requirements for #{names[0..-2].join(', ')} and #{names[-1]}"
names = dependencies.map(&:name).uniq
if names.count == 1
"requirements for #{names.first}"
else
"requirements for #{names[0..-2].join(', ')} and #{names[-1]}"
end
Comment on lines +82 to +86
Copy link
Copy Markdown
Contributor

@landongrindheim landongrindheim Oct 19, 2022

Choose a reason for hiding this comment

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

💡 Since we have ActiveSupport available, we could use #to_sentence for this case (docs)

Copy link
Copy Markdown
Contributor

@bdragon bdragon Oct 19, 2022

Choose a reason for hiding this comment

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

I had this change stashed so I just pushed it up here in case you want to use any of it: ce8eb68

Copy link
Copy Markdown
Contributor Author

@mctofu mctofu Oct 19, 2022

Choose a reason for hiding this comment

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

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.

end
end

# rubocop:disable Metrics/AbcSize
def application_pr_name
pr_name = "bump "
pr_name = pr_name.capitalize if pr_name_prefixer.capitalize_first_word?
Expand All @@ -104,10 +109,15 @@ def application_pr_name
"#{from_version_msg(previous_version(dependency))}" \
"to #{new_version(dependency)}"
else
names = dependencies.map(&:name)
"#{names[0..-2].join(', ')} and #{names[-1]}"
names = dependencies.map(&:name).uniq
if names.count == 1
names.first
else
"#{names[0..-2].join(', ')} and #{names[-1]}"
end
Comment on lines +113 to +117
Copy link
Copy Markdown
Contributor

@landongrindheim landongrindheim Oct 19, 2022

Choose a reason for hiding this comment

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

Same thought here re: #to_sentence 🙂

end
end
# rubocop:enable Metrics/AbcSize

def pr_name_prefix
pr_name_prefixer.pr_name_prefix
Expand Down
108 changes: 100 additions & 8 deletions common/spec/dependabot/pull_request_creator/message_builder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,19 @@ def commits_details(base:, head:)
end

context "with two dependencies" do
let(:dependencies) { [dependency, dependency] }
it { is_expected.to eq("Bump business and business") }
let(:dependency2) do
Dependabot::Dependency.new(
name: "business2",
version: "1.5.0",
previous_version: "1.4.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependencies) { [dependency, dependency2] }

it { is_expected.to eq("Bump business and business2") }

context "for a Maven property update" do
let(:dependency) do
Expand Down Expand Up @@ -297,9 +308,45 @@ def commits_details(base:, head:)
end
end

context "with two dependencies with the same name" do
let(:dependency2) do
Dependabot::Dependency.new(
name: "business",
version: "2.3.0",
previous_version: "2.1.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependencies) { [dependency, dependency2] }
it { is_expected.to eq("Bump business") }
end

context "with three dependencies" do
let(:dependencies) { [dependency, dependency, dependency] }
it { is_expected.to eq("Bump business, business and business") }
let(:dependency2) do
Dependabot::Dependency.new(
name: "business2",
version: "1.5.0",
previous_version: "1.4.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependency3) do
Dependabot::Dependency.new(
name: "business3",
version: "1.5.0",
previous_version: "1.4.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependencies) { [dependency, dependency2, dependency3] }

it { is_expected.to eq("Bump business, business2 and business3") }
end

context "with a directory specified" do
Expand Down Expand Up @@ -643,20 +690,65 @@ def commits_details(base:, head:)
end

context "with two dependencies" do
let(:dependencies) { [dependency, dependency] }
let(:dependency2) do
Dependabot::Dependency.new(
name: "business2",
version: "1.5.0",
previous_version: "1.4.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependencies) { [dependency, dependency2] }

it "includes both dependencies" do
expect(pr_name).
to eq("Update requirements for business and business")
to eq("Update requirements for business and business2")
end
end

context "with two dependencies with the same name" do
let(:dependency2) do
Dependabot::Dependency.new(
name: "business",
version: "2.3.0",
previous_version: "2.1.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependencies) { [dependency, dependency2] }
it { is_expected.to eq("Update requirements for business") }
end

context "with three dependencies" do
let(:dependencies) { [dependency, dependency, dependency] }
let(:dependency2) do
Dependabot::Dependency.new(
name: "business2",
version: "1.5.0",
previous_version: "1.4.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependency3) do
Dependabot::Dependency.new(
name: "business3",
version: "1.5.0",
previous_version: "1.4.0",
package_manager: "dummy",
requirements: [],
previous_requirements: []
)
end
let(:dependencies) { [dependency, dependency2, dependency3] }

it "includes all three dependencies" do
expect(pr_name).
to eq("Update requirements for business, business and business")
to eq("Update requirements for business, business2 and business3")
end
end

Expand Down