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.

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

Disallow class-level warning suppression #411

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Disallow class-level warning suppression #411

wants to merge 1 commit into from

Conversation

findepi
Copy link
Contributor

@findepi findepi commented May 29, 2024

@SuppressWarnings should be applied to narrowest scope possible.

@findepi findepi requested review from electrum and wendigo May 29, 2024 14:59
@wendigo
Copy link
Contributor

wendigo commented May 29, 2024

Did you try that on the Trino?

@findepi findepi marked this pull request as draft May 29, 2024 15:20
@findepi
Disallow class-level warning suppression
e69a1df 
`@SuppressWarnings` should be applied to narrowest scope possible.
@findepi findepi marked this pull request as ready for review May 29, 2024 15:55
@findepi
Copy link
Contributor Author

findepi commented May 29, 2024

Did you try that on the Trino?

trinodb/trino#22193

wendigo
wendigo approved these changes Jun 2, 2024
View reviewed changes
Copy link
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

LGTM but you'll need to apply it in Trino and Airlift :)

@wendigo
Copy link
Contributor

wendigo commented Jun 3, 2024

Sometimes these suppressions are actually needed. In Airlift:

@SuppressWarnings("checkstyle:TypeName")
    public static class $$RecordedConfigData<T>

@findepi
Copy link
Contributor Author

findepi commented Jun 3, 2024

I am not convinced this is how we would name a class in 2024.

@electrum
Copy link
Member

electrum commented Jun 5, 2024

This seems heavy-handed

@findepi
Copy link
Contributor Author

findepi commented Jun 5, 2024

I can't disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wendigo wendigo wendigo approved these changes

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@findepi @wendigo @electrum