Enable suppressing Nodef for specifik code locations

[demus-nine]

No description provided.

[apryamostanov]

Issue: #15
Plugin PR: #16
Linked plugin PR: #14
Correspondent library PR: virtualdogbert/enterprise-groovy#2

[apryamostanov]

<continuing discussion of #14 >

Hi @demus-nine !

First of all thank you very much for the contribution and the idea. Great job!
The technical part is OK from my side, I reviewed the code changes.

However as a a new user of this feature (3rd pair of eyes here), I have two observations:

1. In the configuration.adoc we should provide a specific use case example for the users, i.e.:

---

allowedSuppressions : ["unused", "SpringJavaInjectionPointsAutowiringInspection"]

in conventions.groovy will respectively allow:

@SuppressWarnings('SpringJavaInjectionPointsAutowiringInspection')

and

@SuppressWarnings('unused')

in the client source code.

---

2. We discussed with @virtualdogbert and we both suggest creating a separate annotation @DefAllowed rather than using @SuppressWarnings('NoDef'), as it's more of a compilation error rather than a warning.
   What is your view on this? Will it be possible?

[demus-nine]

If you're using codenarc with NoDef enabled, you would probably be suppressing NoDef at those particular locations anyway, so another annotation would add duplication at all these locations. SuppressWarning was probably named thus for historical reasons, because hitting any codenarc violation most certainly fails the build (in gradle, I don't know how codenarc behaves in other build systems).

You could do a hybrid solution with both a dedicated annotation and hardcoding the common suppressions that are used to allow def/dynamic type while removing the configurability.

I would probably find it annoying to have to duplicate the suppression everywhere it's needed, but YMMV.
The main goal is to allow exceptions where they are unavoidable without having to sacrifice the aggressive checking itself. It found a bunch of garbage in the code I was upgrading, so it's usefulness is indubitable.

[virtualdogbert]

If you're using Enterprise Groovy with defAllowed=false, and codenarc, I think it would make sense to disable or not include the NoDef rule if you were using codenarc, because you're effectively checking twice. I do have sympathy for garbage code, but if you have a lot of suppression annotations or it this case a defAllowed annotations, that's a code smell and something that should be refactored. In the documentation I do make mention of if you're adding this to a legacy project, you may have to do some whitelisting of classes at least until you can refactor code, otherwise, everything will break. It's definitely easier to start with good practices, but a lot of us don't get the chance and have to clean up after the fact.

I'm not a fan of the SuppressWarning annotation, because you're not suppressing a warning you're suppressing an error. Regardless of historical reasons, I would rather the code be clear and make sense.

[demus-nine]

While always running groovy as @CompileStatic is advantageous, there are some cases where dynamic resolution is unavoidable or even preferable. The example from our code I gave in #14 cannot easily be achieved otherwise. It's one the niceties of having a runtime resolving language.
It's not a question of having the NoDef rule or not, it's a question of re-using the well-known ways of excepting selected locations from enforcement. Otherwise the def checking is more or less superfluous as codenarc can perform the same thing more flexibly, (although it also has it's limitations).
We already had the NoDef rule enabled in codenarc, thus the few places where it was needed to allow no type, it was already suppressed. But now that I think about it, def or dynamic type should be unconditionally prohibited in CompileStatic code, because, AFAIK, the the compiler just replaces it with Object, which changes behaviour fx. in the linked example code.
That would mean it's not equivalent to running codenarc with NoDef enabled.

[virtualdogbert]

Sorry about the delay but it's been a busy summer...

However, I'm still not convinced I should allow SuppressWarning annotation in the codebase. I don't care if it's well known, it's still the wrong annotation, for what it would be doing in this codebase. If the def checking in this plugin is superfluous for you because you are using codenarc, you can turn it off in the config. If you need to whitelist or @CompileDynamic in certain areas, to work around things, that option is available to you too.

So bottom line I won't accept this PR with the SuppressWarning annotation.