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

formatAnnotations() removes line breaks between type annotations and types #1275

Merged
merged 26 commits into from
Sep 14, 2022

Conversation

mernst
Copy link
Contributor

@mernst mernst commented Aug 12, 2022

Type annotations should be on the same line as the type that they qualify.

  @Override
  @Deprecated
  @Nullable @Interned String s;

However, some tools format them incorrectly, like this:

  @Override
  @Deprecated
  @Nullable
  @Interned
  String s;

This pull request adds a formatAnnotations() step to correct the incorrect formatting.

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 for the PR! There are a few documentation bits missing:

I have a grip (noted line above) about backward-compatibility and your giant magic value, I think maybe writing the details docs will help you think about the API a bit.

(Also don't worry about the CodeQL Regex gripes above, I think it's fine)

@mernst mernst changed the title typeAnnotations() removes line breaks between type annotations and types formatAnnotations() removes line breaks between type annotations and types Aug 28, 2022
@mernst
Copy link
Contributor Author

mernst commented Aug 28, 2022

@nedtwigg I believe I have addressed your concerns. Could you take another look at the pull request? Thanks!

continue;
}
lines[i] = "";
lines[i + 1] = line.replaceAll("\\s+$", "") + " " + nextLine.replaceAll("^\\s+", "");

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [a user-provided value](2) may run slow on strings with many repetitions of ' '.
*/
boolean endsWithTypeAnnotation(String unixLine) {
// Remove trailing newline.
String line = unixLine.replaceAll("\\s+$", "");

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [a user-provided value](2) may run slow on strings with many repetitions of ' '.
@nedtwigg nedtwigg enabled auto-merge September 14, 2022 00:18
@nedtwigg
Copy link
Member

Thanks very much for a great PR, sorry it took so long to get merged. This will be released by tomorrow morning!

@mernst
Copy link
Contributor Author

mernst commented Sep 14, 2022

Great, thank you for doing that.

@nedtwigg nedtwigg disabled auto-merge September 14, 2022 04:41
@nedtwigg nedtwigg merged commit 3a1104f into diffplug:main Sep 14, 2022
@nedtwigg
Copy link
Member

Released in plugin-gradle 6.11.0 and plugin-maven 2.26.0.

@mernst mernst deleted the typeAnnotations branch September 14, 2022 16:20
@mernst
Copy link
Contributor Author

mernst commented Sep 14, 2022

FYI, 2.30.0 has not yet shown up at
https://mvnrepository.com/artifact/com.diffplug.spotless/spotless-lib

I wonder whether that is related to the fact that the deploy and ext_deploy CircleCI checks are still yellow (in process) here:
https://github.com/diffplug/spotless/runs/8343619107

@nedtwigg
Copy link
Member

Just a slow sync on the Sonatype side, should be resolved now (#1319).

"UpperBoundBottom",
"UpperBoundLiteral",
"UpperBoundUnknown",
"Value",
Copy link
Contributor

Choose a reason for hiding this comment

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

Which @Value-annotation is this? I've just stumbled over this, as formatAnnotations() removed a newline in my code base for a field annotation org.springframework.beans.factory.annotation.Value, which isn't a type annotation (@Target({ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.ANNOTATION_TYPE})).

Maybe this list should contain the fully-qualified name of the annotations. But yeah, the implementation would be more challenging.

Copy link
Member

Choose a reason for hiding this comment

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

As per the docs, you can use addTypeAnnotation and removeTypeAnnotation to tweak for your case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's referring to lombok.Value. I agree that fully-qualified names are the way to go. This would be trivial in (say) google-java-format -- the mechanism already exists -- but would require much more work within Spotless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1367 changes @Value from a type annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification!

As you mention google-java-format: Have you thought about fixing this issue in the fork palantir-java-format? They seem to be more open for community contributions than Google.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you mention google-java-format: Have you thought about fixing this issue in the fork palantir-java-format? They seem to be more open for community contributions than Google.

This is a good idea.

Is it actively maintained? I see only 3 substantive commits since April, and it has not integrated recent changes (including new features and bug commits) from google-java-format. An example is google/google-java-format@865cff01 from August 2021, which would be necessary.

Copy link
Contributor

@chkpnt chkpnt Oct 13, 2022

Choose a reason for hiding this comment

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

It is well-enough maintained for my decision to use it. :-) (I needed an alternative to google-java-format that doesn't force me to use 100 chars lines and 2-spacing-indentation.)

In January, they cherry-picked several commits from upstream, so I assume they are inclined to keep up with google-java-format.

benkard added a commit to benkard/mulkcms2 that referenced this pull request Jan 14, 2023
…0.0 (mulk/mulkcms2!6)

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.29.0` -> `2.30.0` |

---

### Release Notes

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

### [`v2.30.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#&#8203;2300---2022-09-14)

##### Added

-   `formatAnnotations()` step to correct formatting of Java type annotations.  It puts type annotations on the same line as the type that they qualify.  Run it after a Java formatting step, such as `googleJavaFormat()`. ([#&#8203;1275](diffplug/spotless#1275))

##### Changes

-   Bump default `ktfmt` version to latest `0.39` -> `0.40` ([#&#8203;1312](diffplug/spotless#1312))
-   Bump default `ktlint` version to latest `0.46.1` -> `0.47.1` ([#&#8203;1303](diffplug/spotless#1303))
    -   Also restored support for older versions of ktlint back to `0.31.0`

</details>

---

### Configuration

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

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

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

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- 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