Skip to content

[prone] Apply UnnecessaryDefaultInEnumSwitch #2702

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[prone] Apply UnnecessaryDefaultInEnumSwitch #2702

wants to merge 1 commit into from

Conversation

@Pankraz76
Copy link
Contributor

@Pankraz76 Pankraz76 commented Oct 24, 2025

Pankraz76
Pankraz76 commented Oct 24, 2025
View reviewed changes
Copy link
Contributor Author

@Pankraz76 Pankraz76 left a comment

Choose a reason for hiding this comment

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

questionnaire.

@Pankraz76 Pankraz76 marked this pull request as ready for review October 24, 2025 08:37
@Pankraz76 Pankraz76 force-pushed the UnnecessaryDefaultInEnumSwitch branch 2 times, most recently from 2b70427 to 0914367 Compare October 24, 2025 09:51
nedtwigg
nedtwigg requested changes Oct 27, 2025
View reviewed changes
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

This is great, I didn't know java had exhaustiveness checks now. I have only one minor nit.

lib/src/main/java/com/diffplug/spotless/generic/IndentStep.java Outdated
if (state.type == SPACE) {
builder.append(" ".repeat(numSpaces));
} else if (state.type == TAB) {
builder.append("\t".repeat(Math.max(0, numSpaces / state.numSpacesPerTab)));
Copy link
Member

@nedtwigg nedtwigg Oct 27, 2025

Choose a reason for hiding this comment

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

I don't see a need for the Math.max(0) part.

Copy link
Contributor Author

@Pankraz76 Pankraz76 Oct 28, 2025

Choose a reason for hiding this comment

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

yes thats off topic, as an ide suggestion:

Can be replaced with 'String.repeat()'

If we can replace a whole loop, with one single line of code its a good win reducing the overhead to a minimum:

image

Can we merge under this ciscumsstances, or should we undo?

Copy link
Member

@nedtwigg nedtwigg Oct 30, 2025

Choose a reason for hiding this comment

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

we can do the repeat, just not the Math.max.

Copy link
Contributor Author

@Pankraz76 Pankraz76 Oct 31, 2025

Choose a reason for hiding this comment

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

dont know how to fix it, only can apply the ide suggestion. As this is reoccuring else where just leave as it is and address later on.

@Pankraz76 Pankraz76 requested a review from nedtwigg October 28, 2025 07:19
@Pankraz76 Pankraz76 force-pushed the UnnecessaryDefaultInEnumSwitch branch 2 times, most recently from c996369 to 83d1b0b Compare October 31, 2025 16:51
@Pankraz76
Copy link
Contributor Author

Pankraz76 commented Oct 31, 2025 

Changes have been made to gradle/wrapper/gradle-wrapper.properties by:
    com.diffplug.spotless.openrewrite.SanityCheck
        org.openrewrite.gradle.GradleBestPractices
            org.openrewrite.gradle.MigrateToGradle9
                org.openrewrite.gradle.UpdateGradleWrapper: {version=9.x, addIfMissing=false}
Please review and commit the results.
Estimate time saved: 2h 20m

unrelated rewrite change thats actually an issue imposing dirty state of codebase.

@Pankraz76 Pankraz76 force-pushed the UnnecessaryDefaultInEnumSwitch branch from 83d1b0b to 005d1be Compare November 3, 2025 10:44
Pankraz76
Pankraz76 commented Nov 3, 2025
View reviewed changes
lib/src/main/java/com/diffplug/spotless/biome/BiomeStep.java
private FormatterFunc.Closeable toFunc() {
var runner = new ProcessRunner();
return FormatterFunc.Closeable.of(runner, this::format);
return FormatterFunc.Closeable.of(new ProcessRunner(), this::format);
Copy link
Contributor Author

@Pankraz76 Pankraz76 Nov 3, 2025

Choose a reason for hiding this comment

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

Pankraz76
Pankraz76 commented Nov 3, 2025
View reviewed changes
lib/src/main/java/com/diffplug/spotless/generic/LicenseHeaderStep.java Outdated
throw new IllegalStateException(yearMode.toString());
}
return new Runtime(headerLazy.get(), delimiter, yearSeparator, updateYear, skipLinesMatching);
boolean updateYear = switch (yearMode.get()) {
Copy link
Contributor Author

@Pankraz76 Pankraz76 Nov 3, 2025

Choose a reason for hiding this comment

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

same same

@Pankraz76 Pankraz76 force-pushed the UnnecessaryDefaultInEnumSwitch branch 2 times, most recently from b9e6a6e to 5d67390 Compare November 3, 2025 11:06
Pankraz76
Pankraz76 commented Nov 3, 2025
View reviewed changes
lib/src/main/java/com/diffplug/spotless/LineEnding.java
* Represents the line endings which should be written by the tool.
*/
public enum LineEnding {
// @formatter:off
Copy link
Contributor Author

@Pankraz76 Pankraz76 Nov 3, 2025

Choose a reason for hiding this comment

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

is this something to consider or should it stay be reverted?

lib/src/main/java/com/diffplug/spotless/LineEnding.java
case MAC_CLASSIC: return "\r";
default: throw new UnsupportedOperationException(this + " is a path-specific line ending.");
}
return switch (this) {
Copy link
Contributor Author

@Pankraz76 Pankraz76 Nov 3, 2025

Choose a reason for hiding this comment

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

this code is not touched by spotless anyways had to apply ide to format.

@Pankraz76 Pankraz76 force-pushed the UnnecessaryDefaultInEnumSwitch branch from 5d67390 to 22911af Compare November 3, 2025 11:22
@nedtwigg
Copy link
Member

nedtwigg commented Nov 3, 2025

Please don't force push! I have to start my review completely over when that happens. With automated tools, you are able to generate diffs in less time than it takes reviewers to review them. For security reasons, I have to review every diff carefully.

My goal with Spotless is to not block people who want to contribute, but these refactors are consuming too much of my time right now. Can you maybe open an issue with the remaining improvements you want to make?

@nedtwigg nedtwigg closed this Nov 3, 2025
@nedtwigg nedtwigg mentioned this pull request Nov 3, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nedtwigg nedtwigg Awaiting requested review from nedtwigg

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Pankraz76 @nedtwigg