Test Runic formatting #2564

fredrikekre · 2024-08-28T09:21:50Z

No description provided.

fredrikekre · 2024-10-21T13:53:22Z

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.

mortenpi

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.

mortenpi · 2024-10-27T22:59:59Z

docs/make.jl

@@ -18,6 +18,22 @@ Changelog.generate(
    repo = "JuliaDocs/Documenter.jl",
 )

+const linkcheck_ignore = [


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:

Suggested change

const linkcheck_ignore = [

linkcheck_ignore = [

mortenpi · 2024-10-27T23:01:25Z

src/DocSystem.jl

-
-

Does the formatter remove empty lines? Not opposed to it, just curious where this change came from.

It removes empty lines so that there are at max two emtpy lines between code.

mortenpi · 2024-10-27T23:05:58Z

src/deploydocs.jl

+    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


Did the formatter do this? 😮 I would the automatic change to be just a return in front of open.

No, I just thought this looked better than return open(...) do

mortenpi · 2024-10-27T23:06:54Z

src/docchecks.jl

+function hascurl()
+    try
+        return success(`curl --version`)
+    catch err
+        return false
+    end
+end


This was also by the formatter?

No, formatter did:

hascurl() = ( try success(`curl --version`) catch err false end )

(It doesn't allow single-line block constructs).

mortenpi · 2024-10-27T23:10:44Z

src/docchecks.jl

 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()``."
 )


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()``."""

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

mortenpi · 2024-10-27T23:14:38Z

src/documents.jl

+    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


Suggested change

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

mortenpi · 2024-10-27T23:15:05Z

src/documents.jl

+    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


Did you adjust the comments manually here?

mortenpi · 2024-10-27T23:18:11Z

src/html/HTMLWriter.jl

    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


Is this by the formatter? I would have expected a return in front of the if.

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.

mortenpi · 2024-10-27T23:19:50Z

src/latex/LaTeXWriter.jl

-        \\end{figure}
-        """)
+        _println(
+            io, """


I guess I would actually expect this here:

Suggested change

io, """

io,

"""

Formatter doesn't add line breaks in between items but your suggestion passes the formatter too.

mortenpi · 2024-10-27T23:21:57Z

src/utilities/DOM.jl

+const BLOCK_ELEMENTS = Set(
+    [


I do like the conciseness of "merging" the two parentheses.

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.

mortenpi · 2024-10-27T23:33:24Z

The test failures

MethodError: Cannot convert an object of type Nothing to an object of type String

do imply that somewhere we're maybe too aggressively return-ing nothings now?

fredrikekre · 2024-10-28T15:31:09Z

do imply that somewhere we're maybe too aggressively return-ing nothings now?

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.

fredrikekre mentioned this pull request Aug 28, 2024

Insert explicit return expressions in functions/macros/do-blocks fredrikekre/Runic.jl#48

Merged

mortenpi added Type: Maintenance Skip Changelog Allows the CHANGELOG.md check to pass without edit to the file. labels Sep 1, 2024

Runic v1

2fac371

fredrikekre force-pushed the fe/runic branch from 7dfef4c to 2fac371 Compare October 21, 2024 12:13

mortenpi reviewed Oct 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Runic formatting #2564

Test Runic formatting #2564

fredrikekre commented Aug 28, 2024

fredrikekre commented Oct 21, 2024

mortenpi left a comment

mortenpi Oct 27, 2024

mortenpi Oct 27, 2024

fredrikekre Oct 28, 2024

mortenpi Oct 27, 2024

fredrikekre Oct 28, 2024

mortenpi Oct 27, 2024

fredrikekre Oct 28, 2024

mortenpi Oct 27, 2024

fredrikekre Oct 28, 2024

mortenpi Oct 27, 2024

mortenpi Oct 27, 2024

mortenpi Oct 27, 2024

fredrikekre Oct 28, 2024

mortenpi Oct 27, 2024

fredrikekre Oct 28, 2024

mortenpi Oct 27, 2024

fredrikekre Oct 28, 2024

mortenpi commented Oct 27, 2024

fredrikekre commented Oct 28, 2024

Test Runic formatting #2564

Are you sure you want to change the base?

Test Runic formatting #2564

Conversation

fredrikekre commented Aug 28, 2024

fredrikekre commented Oct 21, 2024

mortenpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortenpi commented Oct 27, 2024

fredrikekre commented Oct 28, 2024