new CatchParameterName Check: to validate names of catch-block parameters only

[romani]

Base on discussion at: #2549 , #2604, #2592

Root of the problem is #2549, where do need to remove support of parameters validation in LocalVariableName Check. Yes that is braking compatibility but Check will become logical.

Unfortunately the Check ParameterName does not validate catch parameters at all, it was hardcoded in is logic till #2290 (not released yet, so we could do revert).

Two ways to resolve the conflicts:
0) support of parameters should be removed from LocalVariableName.

1. two parameters "skipCatchParameter"(already done in ParameterName: new option to skip methods with Override annotation #2290, but not released) and "skipParameter" that will do skip for all other parameters. This will allow user to create two configurations in config and by these options adjust logic to validate parameters by separate format.

2. New Check - CatchParameterName. With hardcoded logic to skip non catch parameters.

I think that juggling by parameters are not human friendly and error prone, especially in such case I can not make name better then "skipParameter" but it is inversion of whole name of check. Yes, we have a lot of cases where all is done by options. Creation of new Check for each case is not good also. So lets vote on what is better.

@MEZk , @mkordas , @rnveach , @Vladlis - please share your opinions.

[MEZk]

@romani
I'm for new option 'skipMethodParameter'.
Is it a good idea to implement new check 'CatchParameterName' with completely the same functionality as ParameterName check has? The difference will be only in one if-else.

[romani]

I'm for new option 'skipMethodParameter'.

We also have parameters in Lambdas.

Is it a good idea to implement new check 'CatchParameterName' with completely the same functionality as ParameterName check has?

it might be not that perfect in code , but for user it will be more clear. Both ways have their pros and cons.

[rnveach]

I find it odd the JLS calls the catch-clause variable a parameter. I never really thought of it as one. If anything I would have said it was a local variable, just like method parameters.

I am for 2, the new CatchParameterName. My reason is, after trying I still can't think of the catch-clause variable as a parameter and would find another check spelling out that they are 2 separate items to be easier to understand.

[Vladlis]

I'm for the second option, because:

1. I agree with @rnveach - the first thing I think about, when I read 'parameter name' is a name of a method or constructor parameter, but not a catch or lambda parameter.
2. 'skipParameter' is not an obvious name.
3. Most likely users will specify different patterns for catch parameters and other parameters, so it will be clearer for them to use different checks. It also will make configuration more readable.

[mkordas]

I'm also for the option 2. I'll try to raise PR today - it would be easier to discuss on real code.

[romani]

@MEZk , as majority voted for new Check, I reverted your changes - c324b05 , sorry this happens sometime. Fortunately we caught it before release.

[MEZk]

@romani
Ok. Newertheless, ParameterName should skip catch parameters by default as discussed above.

[romani]

merged.

[romani]

@mkordas , in future we should not forget to update https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/checks-nonjavadoc-error.xml#L372 for each new Check to keep our regression testing and be safe

[mkordas]

@romani, you are right, thanks for making the update. I think that this config should be part of main repo (test code) or at least we should have integration tests between two repositories. What do you think?

[romani]

This is file of regression testing tool, it should not be in main repo, one day we will just use config from main repo (we are not ready now)

[mkordas]

Yeah, so I think that's it's good idea to keep also regression testing tool in main repo, so that it is easy to use for everyone (e.g. by invoking maven with some special profile).

[romani]

May be, but unlikely. Whole contributions repo was inside main repo, i spend a lot of time simplify main repo for new contributors. But we will see ...