-
Notifications
You must be signed in to change notification settings - Fork 480
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
Test Runic formatting #2564
base: master
Are you sure you want to change the base?
Test Runic formatting #2564
Conversation
Update the PR to what I think will be Runic version 1. I skimmed through it and found one bug which is now fixed. Would be great if someone (@mortenpi ?) could have a look and see if there are anything weird too. |
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 skimmed through the PR and I did not see anything really weird, so generally LGTM.
I did leave some questions and thoughts. The ones where I'm like "I don't 100% like this formatting choice" are more just me thinking out loud. I don't really want to second guess the decisions you made in Runic, and I think the formatting bikeshedding ship there has probably passed, and so I am perfectly happy with the formatter, even if it's not a formatting change I would make myself.
If this PR contains both manual and automated changes (seems like it does), and it has been open for a while with changes to master
in the meanwhile, I am a little concerned merging it as is. Maybe we could open a separate PR that just runs Runic and adds the workflow, to get all the automated changes in? And then resolve the diffs/conflicts with this PR? It would make it easier to spot any issues and bad merges etc, if there are any.
@@ -18,6 +18,22 @@ Changelog.generate( | |||
repo = "JuliaDocs/Documenter.jl", | |||
) | |||
|
|||
const linkcheck_ignore = [ |
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.
No reason for this to be const
, and it actually makes it harder to repeatedly include()
make.jl
if you're making changes to this one:
const linkcheck_ignore = [ | |
linkcheck_ignore = [ |
|
||
|
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.
Does the formatter remove empty lines? Not opposed to it, just curious where this change came from.
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 removes empty lines so that there are at max two emtpy lines between code.
v = open(objects_inv) do input | ||
for line in eachline(input) | ||
if startswith(line, "# Version:") | ||
return strip(line[11:end]) | ||
end | ||
end | ||
error("Invalid $objects_inv: missing or invalid version line") | ||
end | ||
return v |
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.
Did the formatter do this? 😮 I would the automatic change to be just a return
in front of open
.
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.
No, I just thought this looked better than return open(...) do
function hascurl() | ||
try | ||
return success(`curl --version`) | ||
catch err | ||
return false | ||
end | ||
end |
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 was also by the formatter?
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.
No, formatter did:
hascurl() = (
try
success(`curl --version`)
catch err
false
end
)
(It doesn't allow single-line block constructs).
GITHUB_ERROR_ADVICE = ( | ||
"This means automatically finding the source URL link for this package failed. " * | ||
"Please add the source URL link manually to the `remotes` field " * | ||
"in `makedocs` or install the package using `Pkg.develop()``." | ||
"Please add the source URL link manually to the `remotes` field " * | ||
"in `makedocs` or install the package using `Pkg.develop()``." | ||
) |
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 guess I'm not a huge fan of this one.. but I supposed it's consistent with the general rules?
I wonder if this would be a better general pattern (not for the formatter, but when trying to write long single-line string literals):
GITHUB_ERROR_ADVICE = """
This means automatically finding the source URL link for this package failed. \
Please add the source URL link manually to the `remotes` field \
in `makedocs` or install the package using `Pkg.develop()``."""
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.
Yea it just falls out naturally from the "rule set", but I have grown to like it because it makes it easier to see that this is just one thing and not a three-tuple, compare
GITHUB_ERROR_ADVICE = (
"This means automatically finding the source URL link for this package failed. " *
"Please add the source URL link manually to the `remotes` field " *
"in `makedocs` or install the package using `Pkg.develop()``."
)
vs
GITHUB_ERROR_ADVICE = (
"This means automatically finding the source URL link for this package failed. ",
"Please add the source URL link manually to the `remotes` field ",
"in `makedocs` or install the package using `Pkg.develop()``."
)
vs
GITHUB_ERROR_ADVICE = (
"This means automatically finding the source URL link for this package failed. " *
"Please add the source URL link manually to the `remotes` field " *
"in `makedocs` or install the package using `Pkg.develop()``."
)
Slightly more useful if there are more things though:
GITHUB_ERROR_ADVICE = (
"This means automatically finding the source URL link for this package failed. " *
"Please add the source URL link manually to the `remotes` field " *
"in `makedocs` or install the package using `Pkg.develop()``.",
"This means automatically finding the source URL link for this package failed. " *
"Please add the source URL link manually to the `remotes` field " *
"in `makedocs` or install the package using `Pkg.develop()``."
)
etc
pages::Vector{String} # Which pages to include in the index? Set by user. | ||
modules::Vector{Module} # Which modules to include? Set by user. | ||
order::Vector{Symbol} # What order should docs be listed in? Set by user. | ||
build::String # Path to the file where this index will appear. | ||
source::String # Path to the file where this index was written. | ||
elements::Vector # (object, doc, page, mod, cat)-tuple for constructing links. | ||
codeblock::MarkdownAST.CodeBlock # original code block |
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.
pages::Vector{String} # Which pages to include in the index? Set by user. | |
modules::Vector{Module} # Which modules to include? Set by user. | |
order::Vector{Symbol} # What order should docs be listed in? Set by user. | |
build::String # Path to the file where this index will appear. | |
source::String # Path to the file where this index was written. | |
elements::Vector # (object, doc, page, mod, cat)-tuple for constructing links. | |
codeblock::MarkdownAST.CodeBlock # original code block | |
pages::Vector{String} # Which pages to include in the index? Set by user. | |
modules::Vector{Module} # Which modules to include? Set by user. | |
order::Vector{Symbol} # What order should docs be listed in? Set by user. | |
build::String # Path to the file where this index will appear. | |
source::String # Path to the file where this index was written. | |
elements::Vector # (object, doc, page, mod, cat)-tuple for constructing links. | |
codeblock::MarkdownAST.CodeBlock # original code block |
pages::Vector{String} # Which pages should be included in contents? Set by user. | ||
mindepth::Int # Minimum header level that should be displayed. Set by user. | ||
depth::Int # Down to which level should headers be displayed? Set by user. | ||
build::String # Same as for `IndexNode`s. | ||
source::String # Same as for `IndexNode`s. | ||
elements::Vector # (order, page, anchor)-tuple for constructing links. | ||
codeblock::MarkdownAST.CodeBlock # original code block |
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.
Did you adjust the comments manually here?
if occursin(r"\.(webm|mp4|ogg|ogm|ogv|avi)$", url) | ||
video[:src => url, :controls => "true", :title => alt]( | ||
return video[:src => url, :controls => "true", :title => alt]( | ||
a[:href => url](alt) | ||
) | ||
else | ||
img[:src => url, :alt => alt] | ||
return img[:src => url, :alt => alt] | ||
end |
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.
Is this by the formatter? I would have expected a return
in front of the if
.
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.
Formatter adds return if
. I considered letting the formatter recurse into branches, but then probably it should also add a else return nothing
branch and I didn't really like that (fredrikekre/Runic.jl#52).
After formatting a couple of code bases it is roughly 50/50 where you actually want to return things or if return nothing
after the if
is what is really intended.
\\end{figure} | ||
""") | ||
_println( | ||
io, """ |
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 guess I would actually expect this here:
io, """ | |
io, | |
""" |
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.
Formatter doesn't add line breaks in between items but your suggestion passes the formatter too.
const BLOCK_ELEMENTS = 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 do like the conciseness of "merging" the two parentheses.
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.
Yea I have thought about special-casing when there is just one thing inside. This is a bit similar to the throw(ArgumentError("msg"))
example above.
The test failures
do imply that somewhere we're maybe too aggressively |
Yea, but should be a manual change by me. I like your suggestion with first merging a "raw" formatted patch and then clean up after. |
No description provided.