-
Notifications
You must be signed in to change notification settings - Fork 30
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
🤖 Automatically format the source code files #416
Conversation
bors merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record I find that this overall did not make the code nicer and rather reduces my interest in working with it.
) | ||
deps = Pkg.TOML.parsefile(joinpath(working_directory, package_relpath, "Deps.toml")) | ||
compat = Pkg.TOML.parsefile(joinpath(working_directory, package_relpath, "Compat.toml")) | ||
# First, we construct a Dict in which the keys are the package's | ||
# dependencies, and the value is always false. | ||
dep_has_compat_with_upper_bound = Dict{String, Bool}() | ||
dep_has_compat_with_upper_bound = Dict{String,Bool}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously?
|
||
# We get the `tree_hash` two ways and check they agree, which helps ensures the `subdir` parameter is correct. Two ways: | ||
# 1. By the commit hash in the PR body and the subdir parameter | ||
# 2. By the tree hash in the Versions.toml | ||
|
||
commit_hash = commit_from_pull_request_body(pr) | ||
|
||
local tree_hash_from_commit, tree_hash_from_commit_success | ||
clone_success = load_files_from_url_and_tree_hash(pkg_code_path, package_repo, tree_hash_from_toml) do dir | ||
local tree_hash_from_commit , tree_hash_from_commit_success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has to be a bug in the formatter, right? I have never seen code written like this.
Base.VersionNumber( | ||
typemax(Base.VInt), typemax(Base.VInt), typemax(Base.VInt) | ||
) in r | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a mess.
compressed = Pkg.TOML.parsefile(path) | ||
compressed = convert(Dict{String, Dict{String, Union{String, Vector{String}}}}, compressed) | ||
uncompressed = Dict{VersionNumber, Dict{String,T}}() | ||
compressed = convert(Dict{String,Dict{String,Union{String,Vector{String}}}}, compressed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely not more readable without space after comma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Julia itself now writes it with spaces:
julia> Dict{String, Dict{String, Union{String, Vector{String}}}}
Dict{String, Dict{String, Union{String, Vector{String}}}}
("master_1", "feature_1", "public_3", "New package: Requires v1.0.0", false, false, requires_commit), # FAIL: UUID conflict, repo differs | ||
("master_1", "feature_1", "public_4", "New package: Requires v1.0.0", false, true, requires_commit), # OK: UUID conflict but name and repo match | ||
]) | ||
for ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete loss of overview.
"can be resumed.") | ||
message = string( | ||
"Failed to clone public registry $(repo) for a check against dependency confusion.\n", | ||
"This is an internal issue with the AutoMerge process and has nothing to do with "."the package being registered but requires manual intervention before AutoMerge ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a safe bet that these code lines have never run. I'll grant it that the auto-formatting did something good in this case.
We could choose a different style? JuliaFormatter has its own default style, and also has YASGuide and BlueStyle. (These changes are BlueStyle.) Alternatively, we could just disable the auto-formatter, and revert the formatting PRs (e.g. this PR), and call it a day. |
If everybody else loves it you should probably go with it, but I don't. (It's possible that some other style matches my preferences better but generally I've found that auto-formatting is always too rigid and gets in the way.) |
I use YASGuide at work, so I am used to it and the quirks of autoformatting (which at least is better than trying to follow a style guide manually IMO). But I don't have a preference here, and don't mind not using a style guide at all, or choosing one of these. And I think if it really annoys Gunnar then we shouldn't do it 🙂 . |
) | ||
|
||
function meets_julia_name_check(pkg) | ||
if occursin("julia", lowercase(pkg)) | ||
return false, "Lowercase package name $(lowercase(pkg)) contains the string \"julia\"." | ||
return false, | ||
"Lowercase package name $(lowercase(pkg)) contains the string \"julia\"." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks quite strange.
@@ -118,12 +122,16 @@ function load_package_data(::Type{T}, path::String, versions::Vector{VersionNumb | |||
end | |||
|
|||
function load_deps(depsfile, versions) | |||
r = load_package_data(Base.UUID, depsfile, versions) isa Dict{VersionNumber,Dict{String,Base.UUID}} | |||
r = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks odd.
# Make sure all paths are unique | ||
path_parts = [splitpath(data["path"]) for (_, data) in reg["packages"]] | ||
for i in 1:maximum(length, path_parts) | ||
i_parts = Set( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen something written like this before.
This pull request formats the source code files using the JuliaFormatter package.