search

jakartaee / validation-tck

Jakarta Validation TCK
http://validator.hibernate.org
Apache License 2.0
38 stars 33 forks source link

BVTCK-57 Followup: Avoid the use of @BeforeMethod #91

Closed marko-bekhta closed 7 years ago

marko-bekhta commented 7 years ago

It's a follow up on

As @gunnarmorling mentioned in the comments of https://github.com/beanvalidation/beanvalidation-tck/pull/86#issuecomment-287355830 I've added checkstyle plugin with configs for imports only. Turned out that there were a few violations so I've cleaned those up.

Also with that last commit I've removed the last Before/After's they were used to change Locale.

As enabling other checlstyle rules will require some code modifications (for example NewlineAtEndOfFile is failing in each file :) ), I think it'll be better if done separately from this one. Let me know if that would be useful and if so - I can work on it :)

gsmet commented 7 years ago

@gunnarmorling the issue I see with using checkstyle to avoid the usage of these 2 annotations is that they indeed might be used correctly. Typically, in the locale case, I don't think it's an issue to use them. I don't foresee a lot more of these correct usages though so we might consider this limitation acceptable.

gunnarmorling commented 7 years ago

they indeed might be used correctly. Typically, in the locale case, I don't think it's an issue to use them

One can turn off CS for specific code sections via comments (//CHECKSTYLE.OFF ... //CHECKSTYLE.ON). It's a bit verbose, but I think it's a good trade-off as there should be just a few legitimate uses.

That said, is setting the locale on the client-side actually a legitimate use? Shouldn't it be done within the VM running the tests?

gsmet commented 7 years ago

That said, is setting the locale on the client-side actually a legitimate use? Shouldn't it be done within the VM running the tests?

It sure can. It's probably more correct but also more verbose. I think Marko found a good trade-off (once my remarks are applied) so I would lean towards accepting it. Just wanted to point this out.

gunnarmorling commented 7 years ago

This one still needs more time to be updated to add back @BeforeMethod and add //CHECKSTYLE-OFF for the few locale uses, right?

marko-bekhta commented 7 years ago

@gunnarmorling that's right. I'll be doing this a bit later today.

marko-bekhta commented 7 years ago

I've reworked last commit and added SuppressionCommentFilter and commented out those two imports with //CHECKSTYLE-OFF //CHECKSTYLE-ON. So this PR seems to be complete now.

As for the other checkstyle rules - would it be of use enabling them and making the code comply those rules?

gunnarmorling commented 7 years ago

So this PR seems to be complete now.

Awesome, rebased and applied. Thanks a lot, Marko!

As for the other checkstyle rules - would it be of use enabling them

Not needed from my side. I'm no CheckStyle zealot really. @gsmet?