Allow members annotated with both @ClassRule and @Rule to be static

[rowanhill]

Hi,

This is the change discussed in #793 to allow static members annotated with both @ClassRule and @Rule (whereas currently, all @Rule members must be non-static).

Although the change is relatively small, I've significantly refactored RuleFieldValidator (and renamed it to RuleMemberValidator). I've kept these two steps in separate commits, with one final commit to actually change the behaviour.

My changes include:

• Changing RuleFieldValidator from an enum to a class. I couldn't see why it was an enum: it's values were never used in that sense, and they don't really seem to semantically be an enumeration.
• Pulled the various bits of logic (controlled by flags to the constructor) out into strategy classes. Each such class has default visibility, so it shouldn't look like anythings changed to the world outside the package. These aren't individually tested, as I think the existing tests cover them well enough. I could add more fine grained tests if that's necessary, though.
• Constructed the four validators using a builder, purely for enhanced readability; it's now (hopefully) very easy to look at the validator definition and read off the logic they're validating.
• Renamed RuleFieldValidator to RuleMemberValidator, since we validate both methods and fields
• Added new tests & updated one of the strategies to allow non-static @ClassRule members, if and only if they're also annotated as @Rule.

I've referred to @Rule annotated members as 'test rules' in a few places, as that wording already seemed to be present in the codebase, but I'm not sure if that's strictly correct terminology (since a TestRule can be annotated with @ClassRule).

I think that covers everything - let me know if anyone has questions, or if anything needs altering for this PR to be accepted.

Thanks,
Rowan

[kcooney]

style-nit: we don't start parameter names with 'f'

[kcooney]

Thanks for the contribution. Unfortunately, github doesn't make it easy for me to comment on the refactoring independent from the functional change. I like having the changes separate, but let's worry about doing that once we resolve the issues.

[rowanhill]

Thanks for the detailed review! I've made all but one of your suggested changes (see the inline comment about getAnnotation vs getAnnotations above). I'm never sure whether to amend commits and force push, or append commits and then rebase once everyone's happy: I've done the former here, I hope that's okay.

I was a bit hesitant about putting all the validators into nested classes - I actually originally wrote them like that, but I felt it made the parent class a bit ungainly. It's not so bad now the Builder is a lot smaller, though.

Also note that I've introduced a fourth commit, for making the static MethodRule validation message more precise (so it didn't get lost in the refactoring), and I've added a couple of tests for it.

A couple of small changes I have left in the refactoring (but possibly shouldn't have done):

• One of the error messages had a double space in; I've fixed that, and the corresponding tests
• I've re-worded the must-be-non-static error message very slightly (has to be -> must be)

[kcooney]

style-nit: extra space before @

[rowanhill]

Oops - not sure how that got there. Fixed

[kcooney]

Wow, fantastic turn around! A few minor issues. Don't worry about merging commits at this time (unless it makes it easier for you); let's do it right at the end.

Unfortunately, you have merge conflicts (possibly due to #921). Note that as of that pull, we no longer use the "f" prefix for fields.

[rowanhill]

Okay - I've rebased (and got rid of all the fFields - yay!) and addressed your comments. Since I was rebasing anyway, I've made the changes in the amended commits. I've left notes on each of your comments, explaining what I've done if it wasn't trivial.

[kcooney]

If the choice is between either having a method description or having a @return tag, I always prefer having a method description. The first sentence of the method Javadoc is used in the class overview.

Incidentally, I generally use @code{null} in Javadoc when referring to null

[rowanhill]

Done - it did all seem a bit repetitious :)

[kcooney]

We don't use wildcard imports. They tend to cause problems when new static methods are added to a class

(but thank you very much for adding a test; I wouldn't have bothered because it's trivial, but it's nice to have)

[rowanhill]

Ah, sorry, fixed; I hadn't realised IntelliJ had done that.

[kcooney]

Sweet. Nervously excited that I can merge this tonight

[rowanhill]

Okay, I've updated that last commit. Hopefully that's got it this time - thanks for being patient with me!

[kcooney]

@rowanhill Thank you for being patient! Could you please update the release notes to mention this commit?

[rowanhill]

Sure thing. The release notes are now updated.

Thanks again!

[kcooney]

Thanks. I updated the release notes to warn about static fields/methods annotated with @Rule and running tests in parallel