Skip to content
This repository has been archived by the owner on Jan 27, 2025. It is now read-only.

Agree on a single Checkstyle suppression mechanism #62

Closed
fsteeg opened this issue Nov 5, 2021 · 2 comments · Fixed by #76
Closed

Agree on a single Checkstyle suppression mechanism #62

fsteeg opened this issue Nov 5, 2021 · 2 comments · Fixed by #76

Comments

@fsteeg
Copy link
Member

fsteeg commented Nov 5, 2021

Came up in #60 (comment). I seem to remember that something was not possible with the original checkstyle-disable-line when adding support for @SuppressWarnings in d70d5d3. Maybe I didn't see a way to suppress MultipleStringLiterals for the entire class? See #60 (comment) for other pro/cons of both approaches.

@blackwinter
Copy link
Member

Copying previous discussion for better overview:


Suggesting switching to @SuppressWarnings in ea54a0e, as it's the standard Java suppression mechanism.

Originally posted by @fsteeg in #60 (comment)


But it's also limited in where it can appear! Now the check is suppressed in the whole method org.metafacture.metafix.InterpreterTest.shouldInterpretNested(); should we - unintentionally - add another "magic number", we wouldn't notice.

Also:

  1. At least in our case, check violations are errors, not warnings.
  2. We can't limit the number of allowed exceptions (currently two with checkstyle-disable-line).
  3. The SuppressWithNearbyCommentFilter setting should be removed if we're no longer using it.

Originally posted by @blackwinter in #60 (comment)

@fsteeg
Copy link
Member Author

fsteeg commented Nov 10, 2021

Discussed: switch to checkstyle-disable-line comments for consistency with metafacture-core, disable MultipleStringLiterals checks for tests.

No functional review (refactoring), code review: @blackwinter / @fsteeg

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants