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

Add spotless to actually run on PR and push to main #63

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

maxspier
Copy link
Member

@maxspier maxspier commented Mar 4, 2024

Description

I want spotlessApply to actually run on PR. Maybe this will help?

@dejabot
Copy link
Contributor

dejabot commented Mar 5, 2024

I think this will be a bit more involved, since this would modify the PR, not just run some checks. See https://github.com/orgs/community/discussions/26842.

It might make more sense to integrate this right into VS Code, so code is reformatted on saves. I've asked some students if they want to run with this - feel free to take a look. eg, we could simply use https://marketplace.visualstudio.com/items?itemName=richardwillis.vscode-spotless-gradle (and even make that a suggested extension in our source tree). or could write something that adds spotlessApply to the VS Code command palette. @JayAgra has had some thoughts about this too.

@dejabot
Copy link
Contributor

dejabot commented Mar 5, 2024

alternate that keeps things (incl any changes that need to be committed) on the client: #65

@dejabot
Copy link
Contributor

dejabot commented Mar 7, 2024

do you still want to pursue this? I expect it will require changes along the lines of what I referenced, so the server can make changes/commits itself.

personally, I'd feel more comfortable with developers be aware of any changes they commit eg through VS Code. curious about your thoughts.

@JayAgra
Copy link
Member

JayAgra commented Mar 7, 2024

@JayAgra has had some thoughts about this too.

indeed i do. i definitely agree that a branch's content shouldn't be automatically changed, but this comes from a distrust of tools that i am not 100% familiar with the workings of. the extension is probably the best bet.

@Mixmix00 is it not enough to have it be part of the build task? wouldn't you verify that your code builds before pushing/opening a PR anyways?

@jamesi8086
Copy link
Contributor

do you still want to pursue this? I expect it will require changes along the lines of what I referenced, so the server can make changes/commits itself.

personally, I'd feel more comfortable with developers be aware of any changes they commit eg through VS Code. curious about your thoughts.

Really depends on the goal; at work we actually have auto formatters on commit.
But from an education POV, it does not really help when students rely on it too much.

That said, as we look into the future, perhaps manual formatting need not necessarily be a thing anymore. Its really comes down to ideology.

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