run automerge on malformed package names, but exit early (#550)

* run automerge on malformed package names, but exit early

* fix guideline (should return message also)

* test that we fail the consistency test suite when appropriate

* switch to `early_exit_if_failed`

* fix docs

* Revert "test that we fail the consistency test suite when appropriate"

This reverts commit 86c9fb8.

* fix missed bit from revert

docs/src/guidelines.md

@@ -25,7 +25,7 @@ function guidelines_to_markdown_output(guidelines_function::Function)
         use_distance_check = false,
         package_author_approved = false,
     )
-    filter!(x -> x[1] != :update_status, guidelines)
+    filter!(x -> !(x[1] isa Symbol), guidelines)
     filter!(x -> !(x[1].docs isa Nothing), guidelines)
     docs = [rstrip(x[1].docs) for x in guidelines]
     output_string = join(string.(collect(1:length(docs)), Ref(". "), docs), "\n")
@@ -55,7 +55,7 @@ function guidelines_to_markdown_output(guidelines_function::Function)
         use_distance_check = false,
         package_author_approved = false,
     )
-    filter!(x -> x[1] != :update_status, guidelines)
+    filter!(x -> !(x[1] isa Symbol), guidelines)
     filter!(x -> !(x[1].docs isa Nothing), guidelines)
     docs = [rstrip(x[1].docs) for x in guidelines]
     output_string = join(string.(collect(1:length(docs)), Ref(". "), docs), "\n")
@@ -139,4 +139,4 @@ Specifically, these labels are:
 * `Override AutoMerge: package author approved`
     * This label can be manually applied, but typically is applied by a separate Github Actions workflow which monitors the PR for comments by the package author, and applies this label if they write `[merge approved]`.
     * This label currently only skips the "sequential version number" check in new versions. In the future, the author-approval mechanism may be used for other checks (on both "new version" registrations and also "new package" registrations).
-        * When AutoMerge fails a check that can be skipped by author-approval, it will mention so in the comment, and direct authors to comment `[merge approved]` if they want to skip the check.
+        * When AutoMerge fails a check that can be skipped by author-approval, it will mention so in the comment, and direct authors to comment `[merge approved]` if they want to skip the check.

src/AutoMerge/guidelines.jl

@@ -429,6 +429,22 @@ function perform_distance_check(labels)
     return true
 end
 
+const guideline_name_identifier = Guideline(;
+    info="Name is a Julia identifier",
+    docs=string(
+        "The package name should be a valid Julia identifier (according to `Base.isidentifier`).",
+    ),
+    check=data -> meets_name_is_identifier(data.pkg),
+)
+
+function meets_name_is_identifier(pkg)
+    if Base.isidentifier(pkg)
+        return true, ""
+    else
+        return false, "The package's name ($pkg) is not a valid Julia identifier according to `Base.isidentifier`. Typically this means it contains `-` or other characters that can't be used in defining a variable name or module. The package must be renamed to be registered."
+    end
+end
+
 const guideline_normal_capitalization = Guideline(;
     info="Normal capitalization",
     docs=string(
@@ -996,7 +1012,7 @@ function _run_pkg_commands(
     @info(string(
         "IMPORTANT: If you see any messages of the form \"Error: Some registries failed to update\"",
         "or \"registry dirty\", ",
-        "please disregard those messages. Those messages are normal and do not indicate an error.", 
+        "please disregard those messages. Those messages are normal and do not indicate an error.",
     ))
     cmd_ran_successfully = success(pipeline(cmd; stdout=stdout, stderr=stderr))
     cd(original_directory)
@@ -1025,6 +1041,10 @@ function get_automerge_guidelines(
     package_author_approved::Bool # currently unused for new packages
 )
     guidelines = [
+        # We first verify the name is a valid Julia identifier.
+        # If not, we early exit (`:early_exit_if_failed`), since we don't want to proceed further.
+        (guideline_name_identifier, true),
+        (:early_exit_if_failed, true),
         (guideline_registry_consistency_tests_pass, true),
         (guideline_pr_only_changes_allowed_files, true),
         # (guideline_only_changes_specified_package, true), # not yet implemented

src/AutoMerge/pull_requests.jl

@@ -1,4 +1,4 @@
-const new_package_title_regex = r"^New package: (\w*?) v(\S*?)$"
+const new_package_title_regex = r"^New package: (\S*) v(\S*)$"
 
 const new_version_title_regex = r"^New version: (\w*?) v(\S*?)$"
 
@@ -203,7 +203,9 @@ function pull_request_build(data::GitHubAutoMergeData; check_license, new_packag
 
     for (guideline, applicable) in guidelines
         applicable || continue
-        if guideline == :update_status
+        if guideline == :early_exit_if_failed
+            all(passed, checked_guidelines) || break
+        elseif guideline == :update_status
             if !all(passed, checked_guidelines)
                 update_status(
                     data;

test/automerge-integration.jl

@@ -166,6 +166,16 @@ hello_world_commit2 = "57b0aec49622faa962c6752d4bc39a62b91fe37c"
             true,   # pass
             requires_commit,
         ),   # OK: UUID conflict but name and repo match
+        (
+            "master_1",
+            "feature_9",
+            "",
+            "New package: Requires-dash v1.0.0",
+            true,   # point_to_slack
+            true,   # check_license
+            false,   # pass
+            requires_commit,
+        ), # FAIL: new package name is not a Julia identifier
     ])
         @info "Performing integration tests with settings" test_number master_dir feature_dir public_dir title point_to_slack check_license pass commit
         with_master_branch(

test/automerge-unit.jl

@@ -120,6 +120,10 @@ end
 end
 
 @testset "Guidelines for new packages" begin
+    @testset "Package name is a valid identifier" begin
+        @test AutoMerge.meets_name_is_identifier("Hello")[1]
+        @test !AutoMerge.meets_name_is_identifier("Hello-GoodBye")[1]
+    end
     @testset "Normal capitalization" begin
         @test AutoMerge.meets_normal_capitalization("Zygote")[1]  # Regular name
         @test AutoMerge.meets_normal_capitalization("Zygote")[1]
@@ -404,6 +408,11 @@ end
                 @test occursin(
                     AutoMerge.new_package_title_regex, "New package: HelloWorld v1.2.3"
                 )
+                # This one is not a valid package name, but nonetheless we want AutoMerge
+                # to run and fail.
+                @test occursin(
+                    AutoMerge.new_package_title_regex, "New package: Mathieu-Functions v1.0.0"
+                )
                 @test occursin(
                     AutoMerge.new_package_title_regex, "New package: HelloWorld v1.2.3+0"
                 )

test/templates/feature_9/R/Requires-dash/Compat.toml

@@ -0,0 +1,3 @@
+[1]
+julia = "1"
+UUIDs = "1"

test/templates/feature_9/R/Requires-dash/Deps.toml

@@ -0,0 +1,2 @@
+["1"]
+UUIDs = "cf7118a7-6976-5b1a-9a39-7adc72f591a4"

test/templates/feature_9/R/Requires-dash/Package.toml

@@ -0,0 +1,3 @@
+name = "Requires-dash"
+uuid = "ae029012-a4dd-5104-9daa-d747884805df"
+repo = "https://github.com/MikeInnes/Requires-dash.jl.git"

test/templates/feature_9/R/Requires-dash/Versions.toml

@@ -0,0 +1,2 @@
+["1.0.0"]
+git-tree-sha1 = "999513b7dea8ac17359ed50ae8ea089e4464e35e"

test/templates/feature_9/Registry.toml

@@ -0,0 +1,8 @@
+name = "ExampleRegistry"
+uuid = "4ad491bb-ba2e-4d0a-a074-3ebed9b9859b"
+repo = ""
+
+description = "This is a test registry for the AutoMerge integration tests."
+
+[packages]
+ae029012-a4dd-5104-9daa-d747884805df = { name = "Requires-dash", path = "R/Requires-dash" }