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

Automatically suggest fixes for bad formatting #53

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

greimel
Copy link
Collaborator

@greimel greimel commented Sep 18, 2024

@adrhill adrhill merged commit 0d18e3d into JuliaPluto:main Sep 18, 2024
4 checks passed
@greimel greimel deleted the format branch September 18, 2024 11:14
greimel added a commit to greimel/PlutoTeachingTools.jl that referenced this pull request Sep 18, 2024
@greimel
Copy link
Collaborator Author

greimel commented Sep 18, 2024

image

I tested this PR in #51 – it turns out that there is an issue with permissions.

@adrhill do you have permission to adjust the Github access token?

@greimel
Copy link
Collaborator Author

greimel commented Sep 18, 2024

xref https://julialang.slack.com/archives/CPWJ5DGG1/p1726659724707129

according to @JoshuaLampert on Slack, this action will only work on PRs which is not from a fork :-/

@JoshuaLampert
Copy link

For reference: JuliaDiff/ChainRulesCore.jl#489

@adrhill
Copy link
Collaborator

adrhill commented Sep 18, 2024

So should we revert this change and set verbose=true in the JuliaFormatter test?

@testset "Code formatting" begin
@test JuliaFormatter.format(PlutoTeachingTools; verbose=false, overwrite=false)
end

@greimel
Copy link
Collaborator Author

greimel commented Sep 18, 2024

I think it's a good idea to change to verbose=true.

Also, it doesn't hurt to keep this PR. At least you, @eford and others with write-access get better feedback from the formatter.

@eford
Copy link
Collaborator

eford commented Sep 18, 2024

I was also frustrated by getting an error, but having no clue what was causing it or how to fix it. So I like the idea of making sure that any CI errors due to formatting are accompanied by explicit instructions that are useful even a domain expert who is not an expert at software development patterns. But I don't have time to test now, so I'll hope it's fixed and return to it if I encounter another unhelpful error message in the future.

@adrhill
Copy link
Collaborator

adrhill commented Sep 18, 2024

Sorry, I didn't expect this formatting test to cause so much trouble.
I'm completely fine with removing it from the test entirely. Maintainers can still occasionally make separate formatting commits.

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.

4 participants