[GitHub] [geode] kirklund opened pull request #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["kirklund (GitHub)" <gi...@apache.org>]

Also, minor cleanup to remove warnings and/or follow best practices.


[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on issue #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["kirklund (GitHub)" <gi...@apache.org>]

@mcmellawatt Should I merge this change to develop?

[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["kirklund (GitHub)" <gi...@apache.org>]

The real reason here is subtle: I do not use final on classes or methods because it prevents mocking. True, we're never going to mock a private static inner-class, but again, this becomes an example that other developers start to copy. Adding `final` to this inner-class does not provide any value. And in fact it actually increases the amount of code and it adds _noise_ to the code. Noise causes a delay when a developer studies the code. So you have to ask: if I add this to the code, does it provide any value including being more easy to maintain or quicker to understand. I try to minimize noise in every class I touch. I also try to make the class/test follow the best practices and coding conventions that are in the books, articles and examples. We also have a best practice or coding standard in Geode to not make methods or classes final unless there's a very strong reason to do so. This was on the dev-list a few years ago, when I proposed avoiding the use of final on classes and meth
 ods and removing final from all class and method declarations to help facilitate using Mockito. There was a large commit that removed final from these declarations across the entire code base.

The only places of value to use `final` in Geode are: 1) on member fields, 2) (this is much less useful than `#1`) parameters passed into a method, 3) class in User API that must never be extended. The only reason for `#2` is because if we're consistent, then it gives immediate information to a developer that the parameter is never reassigned, and even then it doesn't have much value if the method is small (as all methods should be). As far as I know we have no examples of `#3`.

I think that the reason people keep reintroducing `final` in weird places is because they're using other code in the codebase as an example and we never did a sweep to clean it up.

[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund closed pull request #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["kirklund (GitHub)" <gi...@apache.org>]

[ pull request closed by kirklund ]

[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["mcmellawatt (GitHub)" <gi...@apache.org>]

Is there some reason that this can no longer be final?  Looks like we still instantiate and assign in-line.

[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["mcmellawatt (GitHub)" <gi...@apache.org>]

Again, is there now a restriction where this can no longer be final?  If so, it isn't obvious to me...

[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["kirklund (GitHub)" <gi...@apache.org>]

I'm just following JUnit convention. You'll find that none of the books, articles or examples mark the rules as `final`. It's ok for it to be `final` but I strive for my code to look as much like the books, articles and examples so as not to slow down someone who is reading the code or is looking at the code as an example. Put final rules in one test and you'll see developers start to spread that to other tests. Again, it's not that it's not ok, it's just that if we want to follow the best practices and coding conventions that are put forth in books, articles and other examples, then we should not put `final` on JUnit rules.

[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2978: GEODE-6143: Import methods from Mockito instead of PowerMockito

["mcmellawatt (GitHub)" <gi...@apache.org>]

This is really interesting.  Thank you for going into detail on the history here.  I'm glad we do take a stance to use `final` in the cases you listed, and it makes sense to me why we would not use `final` on things that potentially need to be mocked.  I wonder if we could add a spotless rule for this if we really want to codify it.

[ Full content available at: https://github.com/apache/geode/pull/2978 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org