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

Remove code formatting test #54

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Remove code formatting test #54

merged 3 commits into from
Sep 19, 2024

Conversation

adrhill
Copy link
Collaborator

@adrhill adrhill commented Sep 18, 2024

Adresses #48 (comment) and #53.

@greimel
Copy link
Collaborator

greimel commented Sep 18, 2024

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
Copy link
Collaborator

eford commented Sep 18, 2024

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
Copy link
Collaborator

eford commented Sep 18, 2024

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
Copy link
Collaborator Author

adrhill commented Sep 18, 2024

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 domluna/JuliaFormatter.jl#871 to discuss it. If it gets addressed, we can add the test back in here.

@greimel
Copy link
Collaborator

greimel commented Sep 18, 2024

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

Good point.

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

@adrhill
Copy link
Collaborator Author

adrhill commented Sep 18, 2024

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
Copy link
Collaborator

greimel commented Sep 18, 2024

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
Copy link
Collaborator

greimel commented Sep 18, 2024

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

test/test_linting.jl Outdated Show resolved Hide resolved
@eford
Copy link
Collaborator

eford commented Sep 18, 2024

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

@eford
Copy link
Collaborator

eford commented Sep 18, 2024

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
Copy link
Collaborator

eford commented Sep 18, 2024

Are all three of us happy merging this PR in?

@adrhill
Copy link
Collaborator Author

adrhill commented Sep 19, 2024

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. 👍

@adrhill adrhill merged commit ca8cca8 into main Sep 19, 2024
3 checks passed
@adrhill adrhill deleted the ah/remove-formatting-test branch September 19, 2024 15:40
Sign up for free to 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.

3 participants