Skip to content

JuliaFormatter & workflow - #482

Merged
oxinabox merged 21 commits into
JuliaDiff:masterfrom
st--:st/format
Oct 8, 2021
Merged

JuliaFormatter & workflow#482
oxinabox merged 21 commits into
JuliaDiff:masterfrom
st--:st/format

Conversation

@st--

@st--st-- commented Oct 7, 2021

Copy link
Copy Markdown
Contributor

Resolves#462. Supersedes #463.

@st--st-- mentioned this pull request Oct 7, 2021
@codecov-commenter

codecov-commenter commented Oct 7, 2021

Copy link
Copy Markdown

Codecov Report

Merging #482 (ffddfe4) into master (834901e) will increase coverage by 0.07%.
The diff coverage is 82.79%.

Impacted file tree graph

@@ Coverage Diff @@## master #482 +/- ##
==========================================
+ Coverage 92.92% 93.00% +0.07% 
==========================================
Files 15 15 Lines 791 800 +9 ==========================================
+ Hits 735 744 +9 
Misses 56 56 
Impacted FilesCoverage Δ
src/compat.jl44.44% <0.00%> (ø)
src/config.jl100.00% <ø> (ø)
src/ignore_derivatives.jl75.00% <0.00%> (ø)
src/tangent_types/abstract_zero.jl85.00% <33.33%> (ø)
src/tangent_types/notimplemented.jl72.00% <66.66%> (+1.16%)⬆️
src/rule_definition_tools.jl96.27% <82.35%> (+0.09%)⬆️
src/tangent_types/tangent.jl84.37% <84.78%> (ø)
src/accumulation.jl97.22% <100.00%> (ø)
src/projection.jl98.15% <100.00%> (+0.03%)⬆️
src/tangent_arithmetic.jl96.42% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 834901e...ffddfe4. Read the comment docs.

@sethaxen

Copy link
Copy Markdown
Member

I think we want to follow BlueStyle?

By default, JuliaFormatter uses its own style, not BlueStyle. Can you check in a .JuliaFormatter.toml containing just style = "blue" and then re-run JuliaFormatter?

Also, now would be a good time to add the BlueStyle badge to the ReadMe.

@mzgubicmzgubic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did not review the .yml file.

IMO overall this is an improvement in readability, and I am ok to accept formatter dictatorship on a few cases where the current way of writing (e.g. aligning vertically) is more readable.

Comment threadsrc/rule_definition_tools.jl Outdated
Comment threadsrc/rule_definition_tools.jl Outdated
Comment threadtest/rule_definition_tools.jl
Comment threadtest/rule_definition_tools.jl
Comment threadtest/tangent_types/tangent.jl Outdated
Comment thread.github/workflows/format.yml
Comment thread.github/workflows/format.yml

@oxinaboxoxinabox left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks nice work.
We need to look at it again once it has been set to BlueStyle.

@st--

st-- commented Oct 7, 2021

Copy link
Copy Markdown
ContributorAuthor

I think we want to follow BlueStyle?

By default, JuliaFormatter uses its own style, not BlueStyle. Can you check in a .JuliaFormatter.toml containing just style = "blue" and then re-run JuliaFormatter?

Sure. In the end, I think it's more important to have an automatic formatter than what the style is, exactly.:)

@st--

st-- commented Oct 7, 2021

Copy link
Copy Markdown
ContributorAuthor

Also, now would be a good time to add the BlueStyle badge to the ReadMe.

There's one already:
image

@st--
st-- requested a review from oxinaboxOctober 7, 2021 10:30
Comment threadtest/tangent_types/tangent.jl Outdated

@nickrobinson251nickrobinson251 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

personally, i am against having a formatter CI job, because in my experience it is more pain than it is worth (and i say that as both the biggest contributor to BlueStyle and the person who worked with Dom to have it added to JuliaFormatter.jl). But i am not one of the bigger contributors here so am happy enough to be out voted

I am neutral about running the formatter as a one-off over the existing code (it makes style a bit more consistent but interferes with gt blame), but if we are to do that we should use BlueStyle only (whereas this PR currently includes changes that makes the style "worse" (according to BlueStyles) because it also includes changes made first be a different style, so i think those should be removed before this is merged

Comment threadtest/tangent_types/tangent.jl Outdated
@st--

st-- commented Oct 7, 2021

Copy link
Copy Markdown
ContributorAuthor

personally, i am against having a formatter CI job, because in my experience it is more pain than it is worth (and i say that as both the biggest contributor to BlueStyle and the person who worked with Dom to have it added to JuliaFormatter.jl). But i am not one of the bigger contributors here so am happy enough to be out voted

Personally, in my experience the pain from dealing with the autoformatter is much smaller than all the pain from arguments about style that are completely removed by having the autoformatter. But of course also happy for the core maintainers to vote as they see fit!

I am neutral about running the formatter as a one-off over the existing code (it makes style a bit more consistent

We can simply remove the formatter workflow from this PR again and just keep the formatting changes if you want to do that:)

but interferes with git blame),

A bit, but it's easy enough to "blame previous to this commit" (in the GitHub UI even easier than on the commandline):)

but if we are to do that we should use BlueStyle only

I've now removed all the changes from the previous format run, so its current state is BlueStyle changes only.

Comment threadtest/tangent_types/tangent.jl Outdated
Comment threadtest/tangent_types/tangent.jl Outdated
@st--

st-- commented Oct 7, 2021

Copy link
Copy Markdown
ContributorAuthor

@oxinabox@sethaxen it's now properly following BlueStyle. do you want to accept it as is now, or just the format changes minus github action workflow, or is there anything else you'd like to change about it ?

Comment threadsrc/projection.jl Outdated
Co-authored-by: Lyndon White <oxinabox@ucc.asn.au>
Comment threadsrc/projection.jl Outdated
Comment threadsrc/rule_definition_tools.jl Outdated
Comment threadsrc/rule_definition_tools.jl Outdated
Comment on lines +308 to +309
Tangent{Tuple{Float64,Float64}}(4.0, 8.0) ==
Tangent{Tuple{Float64,Float64}}(2.0, 4.0) * 2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this change is required by BlueStyle.
But I don't think it is forbidden either,
It is fine

@oxinabox

Copy link
Copy Markdown
Member

OK, this worked out better than I initially expected seeing the PR.
Good.

I think we can merge this once the outstanding comments are solved.
Of which there are not so many and they are not too bad.

Excellent. Very nice work

@st--
st-- requested a review from oxinaboxOctober 8, 2021 05:36
Comment threadsrc/rule_definition_tools.jl Outdated
Comment thread.github/workflows/format.yml
@st--

st-- commented Oct 8, 2021

Copy link
Copy Markdown
ContributorAuthor

Looks like the reviewdog doesn't actually make the suggestion for how to fix a line (so you could simply accept the review suggestion, instead of having to fix it manually). Not sure why that is - github actions permissions lacking ?

Comment threadsrc/rule_definition_tools.jl Outdated
Comment threadsrc/rule_definition_tools.jl Outdated
@oxinabox
oxinabox merged commit 70f73c5 into JuliaDiff:masterOct 8, 2021
@oxinabox

Copy link
Copy Markdown
Member

thanks

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

check whitespace as part of CI?

7 participants

@st--@codecov-commenter@sethaxen@oxinabox@simeonschaub@nickrobinson251@mzgubic