Skip to content

Conversation

Pankraz76
Copy link
Contributor

No description provided.

@Pankraz76 Pankraz76 force-pushed the fix-CommonStaticAnalysis branch from 74a991e to c79f4ca Compare September 27, 2025 10:58
@Pankraz76 Pankraz76 force-pushed the fix-CommonStaticAnalysis branch from c79f4ca to 1171328 Compare September 27, 2025 11:17
@Pankraz76 Pankraz76 changed the title [openrewrite] add com.diffplug.spotless.SanityCheck [openrewrite] add com.diffplug.spotless.SanityCheck featuring CommonStaticAnalysis Sep 27, 2025
@Pankraz76 Pankraz76 changed the title [openrewrite] add com.diffplug.spotless.SanityCheck featuring CommonStaticAnalysis [openrewrite] add SanityCheck featuring CommonStaticAnalysis Sep 27, 2025
@Pankraz76 Pankraz76 marked this pull request as ready for review September 27, 2025 11:20
@Pankraz76 Pankraz76 force-pushed the fix-CommonStaticAnalysis branch from 1171328 to 30ce140 Compare September 27, 2025 11:21
Vincent Potucek added 2 commits September 27, 2025 13:42
…tor and dereferenced in com.diffplug.spotless.sql.dbeaver.SQLTokensParser.nextToken()

[openrewrite] add `SanityCheck` featuring `CommonStaticAnalysis`
…mmonStaticAnalysis

# Conflicts:
#	lib/src/main/java/com/diffplug/spotless/sql/dbeaver/SQLTokensParser.java
import org.springframework.boot.autoconfigure.SpringBootApplication;

@

Copy link
Contributor Author

@Pankraz76 Pankraz76 Sep 27, 2025

Choose a reason for hiding this comment

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

undo. as test resource.

this passing CI is an issue, right?

Copy link
Member

Choose a reason for hiding this comment

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

CI seems happy, but we should not run this openrewrite tool on src/main/resources I don't think

Copy link
Contributor Author

@Pankraz76 Pankraz76 Sep 28, 2025

Choose a reason for hiding this comment

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

hm yes can exclude this later on as well. Some of the recipes like line endings or headers apply to this as well.

Imho we drive best with convention over configuration principle, adjusting only on breaking changes like this.
Dedicated this file to exclusion.

@Pankraz76 Pankraz76 requested a review from elharo September 27, 2025 11:56
@jbduncan
Copy link
Member

I should try running this or the main branch sooner or later and see how fast it runs compared to before OpenRewrite's introduction.

In my experience, the OpenRewrite Gradle plugin is orders of magnitude slower than Spotless on a small personal project and it doesn't benefit from incremental processing.

But maybe the cost is amortised more on a larger project like Spotless itself. If so, I'd be more than happy to see OpenRewrite's continued use here. Worst case, I could follow up on my earlier effort to make OpenRewrite configuration-cache-compatible, which I believe would make the plugin faster.

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.

okay, whatever you do, do not force-push! this was huge and took forever to review, but it looks good.

I see two issues:

  • we need to get testlib/src/main/resources out of it
  • there's one change in sql/DBeaver which is nonsensical
    • ignore that file
    • or i suppose it's okay to delete that comment

import org.springframework.boot.autoconfigure.SpringBootApplication;

@

Copy link
Member

Choose a reason for hiding this comment

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

CI seems happy, but we should not run this openrewrite tool on src/main/resources I don't think

@Pankraz76 Pankraz76 requested a review from nedtwigg September 28, 2025 10:04
@Pankraz76
Copy link
Contributor Author

how to apply changes like this to maven ? @slachiewicz

Imho we need some tool to achieve this on large scale.

@nedtwigg
Copy link
Member

not sure why exclusing 9c0fab9 caused so many changes, seems like adding just one exclude should cascade through huge changes. rewrite is failing btw.

@Pankraz76
Copy link
Contributor Author

Pankraz76 commented Sep 29, 2025

rebase.

seems ready now.

Thanks for the invest.

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.

lgtm

@nedtwigg nedtwigg merged commit 16f79da into diffplug:main Sep 29, 2025
20 checks passed
@jbduncan
Copy link
Member

Loving the fixes to the various classes to make them final! LGTM. 👍

It's on my TODO list to follow up on the performance side of OpenRewrite.

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