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

Re-lints and addresses most IntelliJ warnings #730

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

HEdingfield
Copy link
Contributor

Fixes #685.

The most notable thing is the removal of ballotTypeId, so please look over that carefully and make sure it makes sense.

Copy link
Contributor

@tarheel tarheel left a comment

Choose a reason for hiding this comment

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

It's not ideal that we end up with this huge formatting commits, because it makes the blame history hard to decipher. Is this happening because @artoonie's workflow doesn't apply the style guide automatically, or it applies slightly different rules? Could we avoid this if we incorporated the linting into a GitHub action?

@artoonie
Copy link
Collaborator

artoonie commented Jul 3, 2023

My workflow applies some of the styleguides but not the intellij one it seems -- I did check the boxes laid out for auto-styling, but those only apply if you commit to git through intellij I think?

IMO it would be good to automate this in github actions if possible.

If this is auto-formatted, I'd also request holding off until the rest of the PRs are merged because this could cause a merge conflict nightmare.

Finally, for these large revision commits, we can preserve git blame history using an --ignore-revs-file in the .git/config file: https://akrabat.com/ignoring-revisions-with-git-blame/

Copy link
Collaborator

@artoonie artoonie left a comment

Choose a reason for hiding this comment

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

all potential conflicts should be merged by now, so approving this!

# Conflicts:
#	src/main/java/network/brightspots/rcv/ContestConfig.java
#	src/main/java/network/brightspots/rcv/RawContestConfig.java
#	src/main/java/network/brightspots/rcv/Tabulator.java
@HEdingfield
Copy link
Contributor Author

My workflow applies some of the styleguides but not the intellij one it seems -- I did check the boxes laid out for auto-styling, but those only apply if you commit to git through intellij I think?

Yes, I think that's the case. I always commit via IntelliJ.

IMO it would be good to automate this in github actions if possible.

Definitely open to this if it's possible! But it would need human review to make the calls on the changes, because much of it is non-trivial, and there are some warnings I purposely ignore.

Finally, for these large revision commits, we can preserve git blame history using an --ignore-revs-file in the .git/config file: https://akrabat.com/ignoring-revisions-with-git-blame/

@tarheel I take your point, but I'm ambivalent about this. Do you guys want me to start one of these files and put this commit in it in this PR, or is it not a big deal? Will hold off on merging until I hear about this.

@tarheel
Copy link
Contributor

tarheel commented Aug 13, 2023

Definitely open to this if it's possible! But it would need human review to make the calls on the changes, because much of it is non-trivial, and there are some warnings I purposely ignore.

The action could add a new commit to the PR. But I guess we'd need a way to add one more commit on top of that reverting the changes that we didn't like without triggering an immediate un-revert commit from the action.

@tarheel I take your point, but I'm ambivalent about this. Do you guys want me to start one of these files and put this commit in it in this PR, or is it not a big deal? Will hold off on merging until I hear about this.

Not clear to me from that link: is the --ignore-revs-file setting reflected on github.com, or only locally? I always browse the blame history on github.com, not using a local tool. Anyway, if it's helpful, it would make sense to apply it to this commit... but in the future we should just try to make commits like this unnecessary.

@artoonie
Copy link
Collaborator

FWIW I wouldn't automate adding a commit, rather I'd just automate a stylecheck (similar to the current style check) that fails if it doesn't follow the styleguide, and it's up to us to fix or ignore as needed.

I would guess that most of the same checks are available in the current style checker, we just need to modify google_checks.xml to match -- though I'm guessing without any investigation.

I can also adjust my workflow to use Intellij git commits, but I always prefer workflow-agnostic solutions.

@tarheel
Copy link
Contributor

tarheel commented Aug 14, 2023

I can also adjust my workflow to use Intellij git commits, but I always prefer workflow-agnostic solutions.

I agree that this is preferable.

@HEdingfield
Copy link
Contributor Author

Per our chat today, going ahead and merging this but filed #743 and #744 to potentially address our discussion here in the future.

@HEdingfield HEdingfield merged commit 8cbafa4 into develop Aug 16, 2023
1 check passed
@HEdingfield HEdingfield deleted the issue-685_code-cleanup branch August 16, 2023 17:08
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.

Update the Google coding format and re-lint everything
3 participants