[openstreetmap/openstreetmap-website] Added workflow for PR labeling using Danger (PR #4988)

[Tom Hughes]

@tomhughes requested changes on this pull request.

In addition to the inline comments, should we update `.rubocop.yml` to add an include for `Dangerfile` so it gets linted/

> +
+# Report if some translation file (except en.yml) is modified
+unless modified_yml_files.empty?
+  modified_files_str = modified_yml_files.map { |file| "`#{file}`" }.join(", ")
+  warn("The following YAML files other than `en.yml` have been modified: #{modified_files_str}. Only `en.yml` is allowed to be changed.")
+  auto_label.set(pr_number, "Compromised Translations", "ff0000")
+end
+
+# Report if there are merge-commits in PR
+are_merge_commits_available = git.commits.any? { |c| c.parents.count > 1 }
+warn("Merge commits found in this pull request. Please, read CONTRIBUTE.md!") if are_merge_commits_available
+auto_label.set(pr_number, "Merge Commits", "ffaec9") if are_merge_commits_available
+
+# Report "Everything is fine!" if no warnings were generated
+message("Everything is fine!") if !is_big_pr && modified_yml_files.empty? && !are_merge_commits_available
+auto_label.set(pr_number, "Good PR", "00ff00") if !is_big_pr && modified_yml_files.empty? && !are_merge_commits_available

Again this could be a block if and I suspect you could use `unless violation_reports.empty?` here rather than retesting all the previous conditions, which will quickly get out of control.

> +auto_label.set(pr_number, "Big PR", "ffff00") if is_big_pr
+
+# Get list of translation files (except en.yml) which are modified
+modified_yml_files = git.modified_files.select do |file|
+  file.start_with?("config/locales") && File.extname(file) == ".yml" && File.basename(file) != "en.yml"
+end
+
+# Report if some translation file (except en.yml) is modified
+unless modified_yml_files.empty?
+  modified_files_str = modified_yml_files.map { |file| "`#{file}`" }.join(", ")
+  warn("The following YAML files other than `en.yml` have been modified: #{modified_files_str}. Only `en.yml` is allowed to be changed.")
+  auto_label.set(pr_number, "Compromised Translations", "ff0000")
+end
+
+# Report if there are merge-commits in PR
+are_merge_commits_available = git.commits.any? { |c| c.parents.count > 1 }

Again it would probably be better to make this a block if, or maybe we should use the `linear_history` plugin? That would give a more generic message though.

> @@ -0,0 +1,32 @@
+# Remove all previously added labels
+auto_label.remove("Big PR")
+auto_label.remove("Compromised Translations")
+auto_label.remove("Merge Commits")
+auto_label.remove("Good PR")
+
+# Report if number of changed lines is > 500
+pr_number = github.pr_json["number"]
+is_big_pr = git.lines_of_code > 500

It would probably be better to make this a block if rather than doing the test and then guarding each line with the same guard.


> @@ -0,0 +1,32 @@
+# Remove all previously added labels
+auto_label.remove("Big PR")
+auto_label.remove("Compromised Translations")
+auto_label.remove("Merge Commits")
+auto_label.remove("Good PR")
+
+# Report if number of changed lines is > 500
+pr_number = github.pr_json["number"]

This is a common variable, not something related to this check, so it should be lifted up above it.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4988#pullrequestreview-2180901032
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4988/review/2180901032 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240716/9bfd167d/attachment.htm>