search

JuliaPluto / PlutoTeachingTools.jl

Functions useful when using Pluto in teaching.
https://juliapluto.github.io/PlutoTeachingTools.jl/example.html
MIT License
62 stars 16 forks source link

Remove code formatting test #54

Closed adrhill closed 1 month ago

adrhill commented 1 month ago

Adresses https://github.com/JuliaPluto/PlutoTeachingTools.jl/pull/48#discussion_r1764812942 and #53.

greimel commented 1 month ago

My suggestion is the following.

  1. Remove formatting from tests. That way, there is a green check if everything else is good
  2. Keep the format check as a separate Github Action. A green check is not required for a PR to be merged
  3. Add this action: https://github.com/julia-actions/julia-format?tab=readme-ov-file#another-possible-workflow-without-julia-format-action. It will create PRs to fix the formatting whenever needed.

As far as I understand it will not bother contributors while working on the PR, but it will create separate PRs that fix the format.

eford commented 1 month ago

I like that idea! It encourages contributions from people who aren't fluent in developer tools, but also makes it easy for us to improve the formatting.

eford commented 1 month ago

But I'm a notice at GitHub workflows, so the only way I know to test things is to merge them, and it often takes me several iterations to get them to work right. So I'd suggest that you two can review each other on this one and decide when to merge.

adrhill commented 1 month ago

Add this action [...]. As far as I understand it will not bother contributors while working on the PR, but it will create separate PRs that fix the format.

I'm not a fan of this. It's basically guaranteed to double the length of the commit history by adding a formatting PR after every commit. If we don't enforce code formatting, we might just as well add a manual formatting commit every so often.

If the formatting test is an issue in this repo, it is an issue everywhere, so I've opened https://github.com/domluna/JuliaFormatter.jl/issues/871 to discuss it. If it gets addressed, we can add the test back in here.

greimel commented 1 month ago

It's basically guaranteed to double the length of the commit history

Good point.

Updated proposal: Do 1. & 2. from above.

adrhill commented 1 month ago

I'm not sure what is gained from (2). If it's a test we don't enforce, why are we testing for it in the first place? It will just put red cross ❌ of CI failure on the main page of the repository, which doesn't signal good development practices to potential users.

greimel commented 1 month ago

The action supposedly provides easy-to-use feedback for all contributors with write access. So at least these contributions will make use of the feedback which leads to a green check.

greimel commented 1 month ago

would you mind adding a commit with bad formatting in this PR - just to see how the feedback of julia-format looks?

eford commented 1 month ago

Why do we care about the length of the commit history?

eford commented 1 month ago

I actually prefer the workflow of accepting contributions (that don't break anything important) even if their formatting doesn't match the pattern. Then someone who is more fluent with the latest software development tools can occassionaly submit a PR that's purely formating. More contributors. Faster merging.

eford commented 1 month ago

Are all three of us happy merging this PR in?

adrhill commented 1 month ago

Why do we care about the length of the commit history?

A neat commit history can function as a change log. It's one of the reasons why many people in the Julia community prefer squash-merging PRs.

I actually prefer the workflow of accepting contributions (that don't break anything important) even if their formatting doesn't match the pattern. Then someone who is more fluent with the latest software development tools can occassionaly submit a PR that's purely formating. More contributors. Faster merging.

I fully agree with this. 👍