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

split out guideline_name_match_check #551

Merged
merged 3 commits into from
Jan 31, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 34 additions & 12 deletions src/AutoMerge/guidelines.jl
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,35 @@ function sqrt_normalized_vd(name1, name2)
return VisualStringDistances.visual_distance(name1, name2; normalize=x -> 5 + sqrt(x))
end

# This check cannot be overridden, since it's important for registry integrity
const guideline_name_match_check = Guideline(;
info = "Name does not match the name of any existing package names (up-to-case)",
docs = "Packages must not match the name of existing package up-to-case, since on case-insensitive filesystems, this will break the registry.",
check=data -> meets_name_match_check(data.pkg, data.registry_master))

function meets_name_match_check(pkg_name::AbstractString, registry_master::AbstractString)
other_packages = get_all_non_jll_package_names(registry_master)
return meets_name_match_check(pkg_name, other_packages)
end

function meets_name_match_check(
pkg_name::AbstractString,
other_packages::Vector;
)
for other_pkg in other_packages
if pkg_name == other_pkg
# We short-circuit in this case; more information doesn't help.
return (false, "Package name already exists in the registry.")
elseif lowercase(pkg_name) == lowercase(other_pkg)
return (false, "Package name matches existing package name $(other_pkg) up-to-case.")
end
end
return (true, "")
end


# This check looks for similar (but not exactly matching) names. It can be
# overridden by a label.
const guideline_distance_check = Guideline(;
info="Name is not too similar to existing package names",
docs="""
Expand Down Expand Up @@ -302,18 +331,9 @@ function meets_distance_check(
)
problem_messages = Tuple{String,Tuple{Float64,Float64,Float64}}[]
for other_pkg in other_packages
if pkg_name == other_pkg
# We short-circuit in this case; more information doesn't help.
return (false, "Package name already exists in the registry.")
elseif lowercase(pkg_name) == lowercase(other_pkg)
# We'll sort this first
push!(
problem_messages,
(
"Package name matches existing package name $(other_pkg) up to case.",
(0, 0, 0),
),
)
if lowercase(pkg_name) == lowercase(other_pkg)
# We handle this case in `meets_name_match_check`
continue
Comment on lines +335 to +336
Copy link
Member

Choose a reason for hiding this comment

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

It seems like it would be fine to just return false here, right? Instead of continuing?

Suggested change
# We handle this case in `meets_name_match_check`
continue
# We can short-circuit in this case.
return (false, "Package name matches existing package name $(other_pkg) up to case.")

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's still nice to show the list of other problems, because it helps tell them what other similar names also exist that they shouldn't use (since they will have to change their name already and might want to choose something similar)

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. If we short-circuit now, then they don't get the full list of other name similarities.

else
msg = ""

Expand Down Expand Up @@ -1022,6 +1042,8 @@ function get_automerge_guidelines(
(guideline_version_can_be_imported, true),
(:update_status, true),
(guideline_dependency_confusion, true),
# this is the non-optional part of name checking
(guideline_name_match_check, true),
# We always run the `guideline_distance_check`
# check last, because if the check fails, it
# prints the list of similar package names in
Expand Down
4 changes: 4 additions & 0 deletions test/automerge-unit.jl
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,10 @@ end
@test !AutoMerge.meets_name_ascii("ábc")[1]
@test AutoMerge.meets_name_ascii("abc")[1]
end
@testset "Package name match check" begin
@test AutoMerge.meets_name_match_check("Flux", ["Abc", "Def"])[1]
@test !AutoMerge.meets_name_match_check("Websocket", ["websocket"])[1]
end
@testset "Package name distance" begin
@test AutoMerge.meets_distance_check("Flux", ["Abc", "Def"])[1]
@test !AutoMerge.meets_distance_check("Flux", ["FIux", "Abc", "Def"])[1]
Expand Down
Loading