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

rubocop: Remove the final Naming/MethodParameterName exceptions: pr #15046

Merged
merged 1 commit into from Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 0 additions & 5 deletions Library/.rubocop.yml
Expand Up @@ -182,15 +182,10 @@ Naming/MethodName:
AllowedPatterns:
- '\A(fetch_)?HEAD\?\Z'

# TODO: remove all of these
Naming/MethodParameterName:
inherit_mode:
merge:
- AllowedNames
AllowedNames:
[
"pr", # TODO: Remove if https://github.com/rubocop/rubocop/pull/11690 is merged or we change the variable names.
]

# Both styles are used depending on context,
# e.g. `sha256` and `something_countable_1`.
Expand Down
38 changes: 22 additions & 16 deletions Library/Homebrew/dev-cmd/pr-pull.rb
Expand Up @@ -82,19 +82,19 @@ def self.separate_commit_message(message)
[subject, body, trailers]
end

def self.signoff!(path, pr: nil, dry_run: false)
def self.signoff!(path, pull_request: nil, dry_run: false)
subject, body, trailers = separate_commit_message(path.git_commit_message)

if pr
if pull_request
# This is a tap pull request and approving reviewers should also sign-off.
tap = Tap.from_path(path)
review_trailers = GitHub.approved_reviews(tap.user, tap.full_name.split("/").last, pr).map do |r|
review_trailers = GitHub.approved_reviews(tap.user, tap.full_name.split("/").last, pull_request).map do |r|
"Signed-off-by: #{r["name"]} <#{r["email"]}>"
end
trailers = trailers.lines.concat(review_trailers).map(&:strip).uniq.join("\n")

# Append the close message as well, unless the commit body already includes it.
close_message = "Closes ##{pr}."
close_message = "Closes ##{pull_request}."
body += "\n\n#{close_message}" unless body.include? close_message
end

Expand Down Expand Up @@ -288,26 +288,26 @@ def self.autosquash!(original_commit, tap:, reason: "", verbose: false, resolve:
raise
end

def self.cherry_pick_pr!(user, repo, pr, args:, path: ".")
def self.cherry_pick_pr!(user, repo, pull_request, args:, path: ".")
if args.dry_run?
puts <<~EOS
git fetch --force origin +refs/pull/#{pr}/head
git fetch --force origin +refs/pull/#{pull_request}/head
git merge-base HEAD FETCH_HEAD
git cherry-pick --ff --allow-empty $merge_base..FETCH_HEAD
EOS
return
end

commits = GitHub.pull_request_commits(user, repo, pr)
commits = GitHub.pull_request_commits(user, repo, pull_request)
safe_system "git", "-C", path, "fetch", "--quiet", "--force", "origin", commits.last
ohai "Using #{commits.count} commit#{"s" unless commits.count == 1} from ##{pr}"
ohai "Using #{commits.count} commit#{"s" unless commits.count == 1} from ##{pull_request}"
Utils::Git.cherry_pick!(path, "--ff", "--allow-empty", *commits, verbose: args.verbose?, resolve: args.resolve?)
end

def self.formulae_need_bottles?(tap, original_commit, user, repo, pr, args:)
def self.formulae_need_bottles?(tap, original_commit, user, repo, pull_request, args:)
return if args.dry_run?

labels = GitHub.pull_request_labels(user, repo, pr)
labels = GitHub.pull_request_labels(user, repo, pull_request)

return false if labels.include?("CI-syntax-only") || labels.include?("CI-no-bottles")

Expand Down Expand Up @@ -352,7 +352,7 @@ def self.changed_packages(tap, original_commit)
formulae + casks
end

def self.download_artifact(url, dir, pr)
def self.download_artifact(url, dir, pull_request)
odie "Credentials must be set to access the Artifacts API" if GitHub::API.credentials_type == :none

token = GitHub::API.credentials
Expand All @@ -362,20 +362,26 @@ def self.download_artifact(url, dir, pr)
# preferred over system `curl` and `tar` as this leverages the Homebrew
# cache to avoid repeated downloads of (possibly large) bottles.
FileUtils.chdir dir do
downloader = GitHubArtifactDownloadStrategy.new(url, "artifact", pr, curl_args: curl_args, secrets: [token])
downloader = GitHubArtifactDownloadStrategy.new(
url,
"artifact",
pull_request,
curl_args: curl_args,
secrets: [token],
)
downloader.fetch
downloader.stage
end
end

def self.pr_check_conflicts(repo, pr)
def self.pr_check_conflicts(repo, pull_request)
long_build_pr_files = GitHub.issues(
repo: repo, state: "open", labels: "no long build conflict",
).each_with_object({}) do |long_build_pr, hash|
next unless long_build_pr.key?("pull_request")

number = long_build_pr["number"]
next if number == pr.to_i
next if number == pull_request.to_i

GitHub.get_pull_request_changed_files(repo, number).each do |file|
key = file["filename"]
Expand All @@ -386,7 +392,7 @@ def self.pr_check_conflicts(repo, pr)

return if long_build_pr_files.blank?

this_pr_files = GitHub.get_pull_request_changed_files(repo, pr)
this_pr_files = GitHub.get_pull_request_changed_files(repo, pull_request)

conflicts = this_pr_files.each_with_object({}) do |file, hash|
filename = file["filename"]
Expand Down Expand Up @@ -463,7 +469,7 @@ def self.pr_pull
autosquash!(original_commit, tap: tap,
verbose: args.verbose?, resolve: args.resolve?, reason: args.message)
end
signoff!(tap.path, pr: pr, dry_run: args.dry_run?) unless args.clean?
signoff!(tap.path, pull_request: pr, dry_run: args.dry_run?) unless args.clean?
end

unless formulae_need_bottles?(tap, original_commit, user, repo, pr, args: args)
Expand Down
30 changes: 15 additions & 15 deletions Library/Homebrew/utils/github.rb
Expand Up @@ -15,10 +15,10 @@ module GitHub

include SystemCommand::Mixin

def self.check_runs(repo: nil, commit: nil, pr: nil)
if pr
repo = pr.fetch("base").fetch("repo").fetch("full_name")
commit = pr.fetch("head").fetch("sha")
def self.check_runs(repo: nil, commit: nil, pull_request: nil)
if pull_request
repo = pull_request.fetch("base").fetch("repo").fetch("full_name")
commit = pull_request.fetch("head").fetch("sha")
end

API.open_rest(url_to("repos", repo, "commits", commit, "check-runs"))
Expand Down Expand Up @@ -194,10 +194,10 @@ def self.search_results_count(entity, *queries, **qualifiers)
json.fetch("total_count", 0)
end

def self.approved_reviews(user, repo, pr, commit: nil)
def self.approved_reviews(user, repo, pull_request, commit: nil)
query = <<~EOS
{ repository(name: "#{repo}", owner: "#{user}") {
pullRequest(number: #{pr}) {
pullRequest(number: #{pull_request}) {
reviews(states: APPROVED, first: 100) {
nodes {
author {
Expand Down Expand Up @@ -289,7 +289,7 @@ def self.upload_release_asset(user, repo, id, local_file: nil, remote_file: nil)
API.open_rest(url, data_binary_path: local_file, request_method: :POST, scopes: CREATE_ISSUE_FORK_OR_PR_SCOPES)
end

def self.get_workflow_run(user, repo, pr, workflow_id: "tests.yml", artifact_name: "bottles")
def self.get_workflow_run(user, repo, pull_request, workflow_id: "tests.yml", artifact_name: "bottles")
scopes = CREATE_ISSUE_FORK_OR_PR_SCOPES

# GraphQL unfortunately has no way to get the workflow yml name, so we need an extra REST call.
Expand Down Expand Up @@ -326,7 +326,7 @@ def self.get_workflow_run(user, repo, pr, workflow_id: "tests.yml", artifact_nam
variables = {
user: user,
repo: repo,
pr: pr.to_i,
pr: pull_request.to_i,
}
result = API.open_graphql(query, variables: variables, scopes: scopes)

Expand All @@ -339,7 +339,7 @@ def self.get_workflow_run(user, repo, pr, workflow_id: "tests.yml", artifact_nam
[]
end

[check_suite, user, repo, pr, workflow_id, scopes, artifact_name]
[check_suite, user, repo, pull_request, workflow_id, scopes, artifact_name]
end

def self.get_artifact_url(workflow_array)
Expand Down Expand Up @@ -542,8 +542,8 @@ def self.check_for_duplicate_pull_requests(name, tap_remote_repo, state:, file:,
end
end

def self.get_pull_request_changed_files(tap_remote_repo, pr)
API.open_rest(url_to("repos", tap_remote_repo, "pulls", pr, "files"))
def self.get_pull_request_changed_files(tap_remote_repo, pull_request)
API.open_rest(url_to("repos", tap_remote_repo, "pulls", pull_request, "files"))
end

def self.forked_repo_info!(tap_remote_repo, org: nil)
Expand Down Expand Up @@ -656,8 +656,8 @@ def self.create_bump_pr(info, args:)
end
end

def self.pull_request_commits(user, repo, pr, per_page: 100)
pr_data = API.open_rest(url_to("repos", user, repo, "pulls", pr))
def self.pull_request_commits(user, repo, pull_request, per_page: 100)
pr_data = API.open_rest(url_to("repos", user, repo, "pulls", pull_request))
commits_api = pr_data["commits_url"]
commit_count = pr_data["commits"]
commits = []
Expand All @@ -677,8 +677,8 @@ def self.pull_request_commits(user, repo, pr, per_page: 100)
end
end

def self.pull_request_labels(user, repo, pr)
pr_data = API.open_rest(url_to("repos", user, repo, "pulls", pr))
def self.pull_request_labels(user, repo, pull_request)
pr_data = API.open_rest(url_to("repos", user, repo, "pulls", pull_request))
pr_data["labels"].map { |label| label["name"] }
end

Expand Down