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

Annotate more format methods in trino-spi #15052

Closed
wants to merge 1 commit into from

Conversation

ksobolew
Copy link
Contributor

Description

The @FormatMethod annotation allows more cases to be checked by the FormatStringAnnotation checker. There are no mis-uses of these methods to fix. All annotated elements are non-public, so the annotation's effects do not leave the module. Except that this requires adding a dependency on error_prone_annotations, and this in turn requires adding it as a provided dependency on all modules which depend on trino-spi and not use this dependency themselves.

Non-technical explanation

Better code quality.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

The `@FormatMethod` annotation allows more cases to be checked by the
`FormatStringAnnotation` checker. There are no mis-uses of these methods
to fix. All annotated elements are non-public, so the annotation's
effects do not leave the module. Except that this requires adding a
dependency on `error_prone_annotations`, and this in turn requires
adding it as a `provided` dependency on all modules which depend on
`trino-spi` and not use this dependency themselves.
@@ -35,6 +35,11 @@
<optional>true</optional>
Copy link
Member

Choose a reason for hiding this comment

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

All annotated elements are non-public,

After a second look, I think we should abandon this PR. It is not going to any plugin implementation and as you have already noticed (with There are no mis-uses) we are solving a problem that do not manifest today. So I think the price of adding new dependency to SPI is too high comparing to benefits.

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 a tough one, yes. We can skip it.

@ksobolew
Copy link
Contributor Author

Closing because changing SPI's dependencies is not trivial.

@ksobolew ksobolew closed this Nov 16, 2022
@ksobolew ksobolew deleted the kudi/format-methods-in-spi branch November 16, 2022 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants