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

WIP: Test Runic formatting #4568

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

fredrikekre
Copy link

@fredrikekre fredrikekre commented Nov 5, 2024

This is a draft PR to try out Runic for code formatting after discussion with @SimonDanisch.

Currently this PR only contains the "raw" Runic formatting with no manual changes, and only for files in src. When applying Runic to other code bases (for example Documenter.jl in JuliaDocs/Documenter.jl#2591 JuliaDocs/Documenter.jl#2564) there are a couple of things one might want to check manually (not for correctness, but for aesthetics).

Runic adds return in front of the last expression in function bodys but this doesn't recurse into try and if, for example. Therefore, one might want to change e.g.

function foo()
    return try
        bar()
    catch
        baz()
    end
end

to

function foo()
    try
        return bar()
    catch
        return baz()
    end
end

depending on what you prefer.

In other cases one might want to be more explicit with what is to be returned (which Runic formatting forces you to do in a sense). For example, in this repo there are many cases of this pattern:

function plot!(p::Plot, args...; kwargs...)
    lines!(p, args...; kwargs...)
end

which after formatting now become

function plot!(p::Plot, args...; kwargs...)
    return lines!(p, args...; kwargs...)
end

The API of plot! is to return p (right?), but here it couples directly to the return value of lines!. Personally I would then rewrite this into

function plot!(p::Plot, args...; kwargs...)
    lines!(p, args...; kwargs...)
    return p
end

just to make it a bit more clear, and to make sure that if the API of lines! changes it doesn't affect the return value of plot!.

If you choose to adopt Runic I think the way we did it in Documenter.jl is a good way: First merge a PR with the "raw" formatting and then make another PR with manual changes just to make it clear what changes are Runic's and what changes are manual. These commits can then be added to a .git-blame-ignore-revs file so that git blame doesn't show these commits, see https://github.com/fredrikekre/Runic.jl?tab=readme-ov-file#ignore-formatting-commits-in-git-blame for details.

The changes here were generated with what I hope to be Runic version 1.0.0. I looked at the patch myself and I didn't see anything I would consider Runic bugs, but I would still be curious if you have any feedback or questions about the actual style.

@SimonDanisch
Copy link
Member

Cool! I only took a look at the first ~10 files, but so far I'm pretty happy with the formatting changes!
I've been trying to be more explicit about returns, so this is great.
Before we can merge this, we'll need to figure out how to rebase the ~100 open PRs, to not destroy the work waiting to be merged.

@fredrikekre fredrikekre force-pushed the fe/runic branch 2 times, most recently from 2667ab4 to 63be8e6 Compare November 6, 2024 14:09
@fredrikekre
Copy link
Author

fredrikekre commented Nov 6, 2024

Before we can merge this, we'll need to figure out how to rebase the ~100 open PRs, to not destroy the work waiting to be merged.

I experimented a bit with this and the easiest seem to be to

git checkout feature-branch-done-before-runic
git rebase -X theirs master

and then run the formatter again and commit the changes.

Edit: If the branch already have conflicts I suppose one should rebase on the commit just before the runic commit just so -X theirs don't apply to all commits up to the runic one.

@ffreyer ffreyer closed this Nov 8, 2024
@ffreyer ffreyer reopened this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Ready to review
Development

Successfully merging this pull request may close these issues.

3 participants