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

Support Rome as a formatter #1663

Merged
merged 36 commits into from
May 17, 2023
Merged

Support Rome as a formatter #1663

merged 36 commits into from
May 17, 2023

Conversation

blutorange
Copy link
Contributor

@blutorange blutorange commented Apr 6, 2023

Add Rome as a formatter for JavaScript and TypeScript code, #1651

A few notes regarding the design:

  • How Rome is located: The NPM and Python/Black formatters currently require that either the binary is available in the path, or that one specifies the binary explicitly in the Gradle / Maven plugin. This is not ideal when the project needs to be compiled in many different environment, such as CI and pipelines, or when there are new developers who want to check out and build a project. We've been working a lot with the frontend-maven-plugin, which does not require any configuration whatsoever. It downloads the NPM binary automatically and uses that locally. So for the Rome integration, I added a similar feature: You can still specifiy the path manually if you want, but by default, the Rome binary is downloaded automatically if it does not exist yet.
    • The Rome download directory, when not given explicitly, defaults to ~/.m2/repository/com/diffplug/spotless/spotless-data ). This is also done by e.g. the dependency check Maven plugin or by the frontend-maven-plugin.
  • How Rome is executed: Currently, I just execute the Rome binary for each request. Rome also offers a language server, which we could start only once and then send commands to. I don't think that's worth it, however. It offers little benefit, since Rome is already pretty fast and does not have any of the startup overhead that e.g. node.JS does have.
    • On my machine (which is not the fastest out there), formatting a short file only takes a couple of milliseconds, formatting the unminified JQuery source code file (about 10000 LOC) takes about 100ms.

TODOs

  • Allow users to use the Rome executable on the path
  • Automatically find the Rome config file and/or add an option to specify the config file
    • This is probably impossible due spotless' caching mechanism? As we can't format depending on the file path, and the config file would have to be derived from the file path.
  • Pass on the config file to the Rome process
  • Support Rome as a formatter for JSON: https://docs.rome.tools/internals/language_support/
  • Support Gradle
    • As a generic format step
    • As a format step for JS/TS/JSON
  • Write docs / README.md
    • Maven
    • Gradle
  • Write tests
    • General
    • Maven
    • Gradle
  • Test on Windows and MacOS (anybody?)

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.

Thanks very much, this looks great! Main feedback is just to shrink the public API as much as possible. Spotless has a ton of formatters, keeping the common API as small as possible and making each formatter as self-contained as possible is important.

CHANGES.md Outdated Show resolved Hide resolved
lib/src/main/java/com/diffplug/spotless/Architecture.java Outdated Show resolved Hide resolved
lib/src/main/java/com/diffplug/spotless/OS.java Outdated Show resolved Hide resolved
lib/src/main/java/com/diffplug/spotless/Platform.java Outdated Show resolved Hide resolved
plugin-gradle/CHANGES.md Outdated Show resolved Hide resolved
plugin-maven/CHANGES.md Outdated Show resolved Hide resolved
@nedtwigg
Copy link
Member

nedtwigg commented Apr 7, 2023

Also, add an entry here

lib('scala.ScalaFmtStep') +'{{yes}} | {{yes}} | {{yes}} | {{no}} |',

And then run spotlessApply to update the table in the root README.

@blutorange
Copy link
Contributor Author

blutorange commented Apr 8, 2023

So this is mostly done now, see here for docs (Maven, Gradle). If anybody has any ideas / suggestions for the configuration, feel free to mention it.

@blutorange
Copy link
Contributor Author

Also, it would be great if somebody could test this on Windows, especially the part where it locates and downloads the Rome binary automatically for the current OS / architecture. Mac OS would also be nice.

@blutorange blutorange marked this pull request as ready for review April 8, 2023 21:57
@blutorange blutorange changed the title Draft: Support Rome as a formatter Support Rome as a formatter Apr 8, 2023
@blutorange
Copy link
Contributor Author

I fixed the spotbugs issues, some NPM tests are failing when I run ./gradlew build -x spotlessCheck locally, but since these are unrelated to these changes, it's probably some issue with my local environment so I hope they'll pass when run via github workflows.

@blutorange
Copy link
Contributor Author

Can the workflow be approved again? With the latest commit it passes on a Windows machine where I tried it

@blutorange
Copy link
Contributor Author

Hmm, those failures in the build pipeline look like a general build issue, the same error also happens on the main branch https://github.com/diffplug/spotless/actions/runs/4934286482/jobs/8819133075

@blutorange
Copy link
Contributor Author

The hash algorithm was set to SHA256, which is supported by some system, but not all. I changed it to SHA-256 which should be supported everywhere.

@nedtwigg nedtwigg enabled auto-merge May 17, 2023 06:22
@nedtwigg
Copy link
Member

Thanks for a great PR and sorry for the slow turnaround. We'll get this merged and released this week.

@blutorange
Copy link
Contributor Author

blutorange commented May 17, 2023

No problem, good things take a while ; ) Also, the build on various different OS's are very helpful to catch issues with path handling etc. There still seems to be an issue creating a path in Windows again for the gradle plugin, where Paths.get needs to be replaced with Path.of so that it handles file URIs correctly.

Edit: seems you already fixed it by removing the slash. Another option is to use Path.of instead of Paths.get

@nedtwigg nedtwigg merged commit 94ba918 into diffplug:main May 17, 2023
@nedtwigg
Copy link
Member

Thanks for the Path.of hint, that's useful!

@nedtwigg
Copy link
Member

Released in plugin-gradle 6.19.0 and plugin-maven 2.37.0.

benkard pushed a commit to benkard/mulkcms2 that referenced this pull request Aug 29, 2023
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.38.0` -> `2.39.0` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.3.0` -> `3.3.1` |
| [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `3.3.0` -> `3.3.1` |

---

### Release Notes

<details>
<summary>diffplug/spotless</summary>

### [`v2.39.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#&#8203;2390---2023-05-24)

##### Added

-   `Jvm.Support` now accepts `-SNAPSHOT` versions, treated as the non`-SNAPSHOT`. ([#&#8203;1583](diffplug/spotless#1583))
-   Support Rome as a formatter for JavaScript and TypeScript code. Adds a new `rome` step to `javascript` and `typescript` formatter configurations. ([#&#8203;1663](diffplug/spotless#1663))
-   Add semantics-aware Java import ordering (i.e. sort by package, then class, then member). ([#&#8203;522](diffplug/spotless#522))

##### Fixed

-   Fixed a regression which changed the import sorting order in `googleJavaFormat` introduced in `2.38.0`. ([#&#8203;1680](diffplug/spotless#1680))
-   Equo-based formatters now work on platforms unsupported by Eclipse such as PowerPC (fixes [durian-swt#&#8203;20](diffplug/durian-swt#20))
-   When P2 download fails, indicate the responsible formatter. ([#&#8203;1698](diffplug/spotless#1698))

##### Changes

-   Equo-based formatters now download metadata to `~/.m2/repository/dev/equo/p2-data` rather than `~/.equo`, and for CI machines without a home directory the p2 data goes to `$GRADLE_USER_HOME/caches/p2-data`. ([#&#8203;1714](diffplug/spotless#1714))
-   Bump default `googleJavaFormat` version to latest `1.16.0` -> `1.17.0`. ([#&#8203;1710](diffplug/spotless#1710))
-   Bump default `ktfmt` version to latest `0.43` -> `0.44`. ([#&#8203;1691](diffplug/spotless#1691))
-   Bump default `ktlint` version to latest `0.48.2` -> `0.49.1`. ([#&#8203;1696](diffplug/spotless#1696))
    -   Dropped support for `ktlint 0.46.x` following our policy of supporting two breaking changes at a time.
-   Bump default `sortpom` version to latest `3.0.0` -> `3.2.1`. ([#&#8203;1675](diffplug/spotless#1675))

</details>

<details>
<summary>quarkusio/quarkus</summary>

### [`v3.3.1`](quarkusio/quarkus@3.3.0...3.3.1)

[Compare Source](quarkusio/quarkus@3.3.0...3.3.1)

</details>

<details>
<summary>quarkusio/quarkus-platform</summary>

### [`v3.3.1`](quarkusio/quarkus-platform@3.3.0...3.3.1)

[Compare Source](quarkusio/quarkus-platform@3.3.0...3.3.1)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox.

👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
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.

3 participants