Community
Participate
Working Groups
https://bugs.eclipse.org/bugs/show_bug.cgi?id=529011#c56 https://bugs.eclipse.org/bugs/show_bug.cgi?id=529011#c60 https://bugs.eclipse.org/bugs/attachment.cgi?id=278206 Could this be a case of something that makes sense logically but is just confusing for a user ? I could definitely see users wondering why the annotation doesn't show up for certain parameter values. Even if it may seem redundant in some cases, it looks more consistent.
Created attachment 278216 [details] Parameter Name Annotations
See https://github.com/angelozerr/jdt-codemining/issues/53 for some background. For me, always showing parameter names made a lot of code so hard to read that i rather turned it off. The current solution is usable, but needs some explanation. Maybe the ui setting label should be "Show parameter names (unless similar to passed values)". Probably this should also be already explained in the N&N page.
FWIW, the interesting pieces of code to decide when to show a codemining parameter name or not is extracted in a dedicated method. We anticipated some possible discussion and room for improvements according to usage feedback, changing it would be easy. The currently implemented rules are not set in stone, and we can obviously change them as best. So, my point of view here, when I chose this rule, is that 1. we should only show codemining when it is likely to provide assistance to end-users and 2. we should assume that the used methods, variables, parameters and so on have good names (it's important that code-mining becomes more a motivation to use better named parameters than being an workaround to poorly name parameters; in good code, code-minings shouldn't have to be printed often because code is explicit enough). Those goals drove to implementing the current rule, that code minings are hidden if the parameter value expression seems to properly map the parameter name. (In reply to Till Brychcy from comment #2) > Maybe the ui setting label should be "Show parameter names (unless similar > to passed values)". That is my favorite approach too. > Probably this should also be already explained in the N&N page. The N&N contains a "umbrelly" statement: "for cases where the resolution may not be obvious for a human rea"
>>"umbrelly" statement: "for cases where the resolution may not be obvious for a I propose addition of "The showing of parameter names is skipped if the parameter value contains the parameter name as its substring."
"similar" and " may not be obvious for a human reader" are too ambiguous. This feature is for anuser who probably knows what an exact match or a substring is.
an user
A user rather
For the wizard, what about "Show method parameter names (unless parameter value contains the parameter name as its substring)" ?
It can be reconsidered if more users find it confusing.
It is so confusing!! It would be (IMHO) much better to always display the parameter names or just ignore them when the value is an exact match to the parameter name rather than a substring.
(In reply to Thiago Berne from comment #10) > It is so confusing!! > It would be (IMHO) much better to always display the parameter names or just > ignore them when the value is an exact match to the parameter name rather > than a substring. Can you please give an example of how this is confusing when reading or writing code? Ie the behavior may be confusing, but is the result itself confusing?
Created attachment 281961 [details] Code Mining is confusing For example the following scenario. There's this method "isBlank" (which is from a third-party lib). It receives a "cs" parameter. And nowhere the code mining is injecting the parameter name even though the values passed are completely different from each other. Other example of confusing scenario is the method "monitorHelper.addValues", which receives a "valuesProvider" parameter. The passed value is "monitorPageChannelValuesProvider" but the parameter name is not injected. It gives me the feeling the parameter name is "monitorPageChannelValuesProvider" (a much more specialized then the real one, "valuesProvider"), wich is not true.
(In reply to Thiago Berne from comment #12) > Created attachment 281961 [details] > Code Mining is confusing > > For example the following scenario. > There's this method "isBlank" (which is from a third-party lib). It receives > a "cs" parameter. And nowhere the code mining is injecting the parameter > name even though the values passed are completely different from each other. > > Other example of confusing scenario is the method "monitorHelper.addValues", > which receives a "valuesProvider" parameter. The passed value is > "monitorPageChannelValuesProvider" but the parameter name is not injected. > It gives me the feeling the parameter name is > "monitorPageChannelValuesProvider" (a much more specialized then the real > one, "valuesProvider"), wich is not true. I don't know anything about that code, and in that situation, I don't have the impression showing the parameter name would increase readability. As explained in comment #3, some decisions were implemented to "guess" when showing a parameter name is useful (make code more readable). At the moment, it still looks like those decisions are correct and your example doesn't contradict that. The main thing that could be changed is the label of the preferences so it would show something like "Show parameter names for potentially ambiguous calls".
(In reply to Mickael Istria from comment #13) > (In reply to Thiago Berne from comment #12) > > Created attachment 281961 [details] > > Code Mining is confusing > > > > For example the following scenario. > > There's this method "isBlank" (which is from a third-party lib). It receives > > a "cs" parameter. And nowhere the code mining is injecting the parameter > > name even though the values passed are completely different from each other. > > > > Other example of confusing scenario is the method "monitorHelper.addValues", > > which receives a "valuesProvider" parameter. The passed value is > > "monitorPageChannelValuesProvider" but the parameter name is not injected. > > It gives me the feeling the parameter name is > > "monitorPageChannelValuesProvider" (a much more specialized then the real > > one, "valuesProvider"), wich is not true. > > I don't know anything about that code, and in that situation, I don't have > the impression showing the parameter name would increase readability. > As explained in comment #3, some decisions were implemented to "guess" when > showing a parameter name is useful (make code more readable). At the moment, > it still looks like those decisions are correct and your example doesn't > contradict that. > The main thing that could be changed is the label of the preferences so it > would show something like "Show parameter names for potentially ambiguous > calls". OK. But I still think it is quite confusing. The fact of the parameter name "valuesProvider" is hidden for the value "monitorPageChannelValuesProvider" makes no sense to me. As I said, it looks like the parameter name is much more specialized then it actually is.
Created attachment 281962 [details] Code Mining is confusing [2] Another example. The constructor TermsAggregation receives as the second parameter a parameter named "field". As the constant passed as parameter have "field" in the path, the parameter name is hidden. It shouldn't be, IMHO. I think it should be, at least, configurable.
(In reply to Thiago Berne from comment #15) > I think it should be, at least, configurable. Contributions welcome ;)
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
I think this is still relevant, at least in 2021-12 (last version I tried - I doubt there's anything new in this area on newer versions, please correct me if I'm wrong). I agree with comment #15: it's not consistent and confusing. At least, I think the heuristics to determine whether the parameter name should be shown or not should consider only variable names and possibly use an exact (perhaps case-insensitive) match, rather than a substring match. It doesn't help when the passed parameter is a constant (like an enum value, whose matching substring may be present on its type, rather than on its name). Anyway, for consistency it would probably be desirable to have an option to completely disable such an heuristics and always show parameter names. But the real killer feature would be, for me, to be able to show parameter names on demand on a piece of code (or single invocation) rather than a complete "all-on" or "all-off" on the whole file. But that's another story.
