Provide name for parameters in 1-arg methods when using code minings #1813

jannisCode · 2024-11-26T14:10:41Z

What it does

This PR will enable the possability to show code minings in all Methods, also those who expect just one argument.

Before:

When a method only had one parameter, it was eligible for code minings, except if it was from a record class. As you can see in the screenshot and snippet, the code minings are shown for car but not for Cat.

import java.util.Objects;

class Main {
	public static void main(String...args) {
		Car c = new Car(3);
		Cat car = new Cat(5);
		car.equals(c);
		car.doSomething(12, 13);
	}
}

 class Cat {
    private int size;

    public Cat(int size) {
        this.size = size;
    }
    public Cat(long age) {}

    public int getSize() {
        return size;
    }

    public void doSomething(int a, int b) {}
    @Override
    public boolean equals(Object obj) {
        if (this == obj) return true;
        if (obj == null || getClass() != obj.getClass()) return false;
        Cat cat = (Cat) obj;
        return size == cat.size;
    }

    @Override
    public int hashCode() {
        return Objects.hash(size);
    }

    @Override
    public String toString() {
        return "Cat{size=" + size + "}";
    }
}

public record Car(int side) {}

@mickaelistria Is there a reason, why it always checks if there are more than one parameter? Because if not, I think it would be beneficial if the code minings are always shown.

After this change

Code minings are always shown no matter how many Parameters there are.

Author checklist

I have thoroughly tested my changes
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

eclipse-jdt-bot · 2024-11-26T14:13:45Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

org.eclipse.jdt.ui/META-INF/MANIFEST.MF
org.eclipse.jdt.ui/pom.xml

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 158338fd1268d2e07ae9ca97d4843c56b2fab3d5 Mon Sep 17 00:00:00 2001
From: Eclipse JDT Bot <[email protected]>
Date: Tue, 26 Nov 2024 14:13:19 +0000
Subject: [PATCH] Version bump(s) for 4.35 stream


diff --git a/org.eclipse.jdt.ui/META-INF/MANIFEST.MF b/org.eclipse.jdt.ui/META-INF/MANIFEST.MF
index 0a5b80724a..d1e2321293 100644
--- a/org.eclipse.jdt.ui/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.ui/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.jdt.ui
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.jdt.ui; singleton:=true
-Bundle-Version: 3.33.200.qualifier
+Bundle-Version: 3.33.300.qualifier
 Bundle-Activator: org.eclipse.jdt.internal.ui.JavaPlugin
 Bundle-ActivationPolicy: lazy
 Bundle-Vendor: %providerName
diff --git a/org.eclipse.jdt.ui/pom.xml b/org.eclipse.jdt.ui/pom.xml
index fffaedcc7e..3818bc298e 100644
--- a/org.eclipse.jdt.ui/pom.xml
+++ b/org.eclipse.jdt.ui/pom.xml
@@ -18,7 +18,7 @@
   </parent>
   <groupId>org.eclipse.jdt</groupId>
   <artifactId>org.eclipse.jdt.ui</artifactId>
-  <version>3.33.200-SNAPSHOT</version>
+  <version>3.33.300-SNAPSHOT</version>
   <packaging>eclipse-plugin</packaging>
 
 	<build>
-- 
2.47.0

Further information are available in Common Build Issues - Missing version increments.

mickaelistria · 2024-11-26T17:04:45Z

Is there a reason, why it always checks if there are more than one parameter?

Back then when I developed this, I found those names useless for single-arg methods. Parameter names are convenient to identify what an argument is intended too; when there is a single arg, the method name is most often sufficient to sort out what's going to happen with the given arg.

fedejeanne · 2024-11-27T05:59:44Z

Is there a reason, why it always checks if there are more than one parameter?

[...] when there is a single arg, the method name is most often sufficient to sort out what's going to happen with the given arg.

Not always, think of constructors

I find having the names for methods with 1 argument is useful and also more consistent with the rest of the behavior. Thank you @jannisCode for this suggestion!

fedejeanne · 2024-11-27T06:09:01Z

...i/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/CalleeJavaMethodParameterVisitor.java

@@ -165,7 +165,7 @@ private boolean skipParameterNameCodeMining(String[] parameterNames, List<?> arg
 	}

 	private boolean skipParameterNamesCodeMinings(IMethod method) {


Remove this method altogether.

Outdated: agreed on adding a parameter which makes this method necessary.

mickaelistria · 2024-11-27T06:34:10Z

Not always, think of constructors

The relevant case to check is when you read code. Even if there are several possible constructors, it's not likely to me that having the parameter name will help readability, eg new Shell(SWT.ON_TOP) or new Shell(Display.getCurrent()) or new Shell(mainShell).... read well without showing the parameter: you rarely wonder what the parameter actually is and what it's used for (unless you poorly name you variables).

more consistent with the rest of the behavior.

Consistency is less important that being useful and lean ;)

I would strongly hate having the parameter names in 1 method args, it would just add noise to me in the very vast majority of use cases, and useless noise hurts readability more than it helps.
So please add a preference to allow users like me to skip such parameter names annotations for those single-args methods.

mickaelistria

Please also provide an option in preferences to skip those.

fedejeanne · 2024-11-29T11:56:49Z

more consistent with the rest of the behavior.

Consistency is less important that being useful and lean ;)

Consistency is important so that users don't end up reporting "bugs" that are not bugs and simply some "other arbitrary behavior". What I meant by that is not that the 1-arg methods don't show the name of the argument but that this behavior is not the same across the board: records don't exhibit this behavior.

Please also provide an option in preferences to skip those.

I'd rather not have such an option, I think the configuration is complex enough. But there are maybe some other opinions on that. Does anyone else (other than Mickael and me) reading this PR care to vote?

mickaelistria · 2024-11-29T12:15:50Z

records don't exhibit this behavior.

That is more a bug to me, to fix separately.

I'd rather not have such an option, I think the configuration is complex enough.

Please make sure you actually try to work with this proposal on various classes before considering it for a merge. My arbitrary choice was based on actual usage of the feature while it was being implemented. My impression based on that experience is that in the vast majority of cases, single arg name is noise and hurts readability.
If several people can actually try it for real, on real code, for some a few days, and report that this the hint are more often useful to them, I will stop arguing; but at the moment, I seem to be the only one who actually experimented that (some years ago) and my feedback is that this is going to hurt readability and would perceived, at least by 1 user, as a regression in usability.
So an option is a must-have to me.

mickaelistria · 2024-12-10T12:49:07Z

...i/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/CalleeJavaMethodParameterVisitor.java

@@ -165,7 +165,7 @@ private boolean skipParameterNameCodeMining(String[] parameterNames, List<?> arg
 	}

 	private boolean skipParameterNamesCodeMinings(IMethod method) {
-		return method.getNumberOfParameters() <= 1;
+		return false;


Please use a preference here

Suggested change

return false;

return PreferenceConstants.getPreferenceStore().getBoolean(PreferencesConstants.EDITOR_JAVA_CODEMINING_SHOW_PARAMETER_NAME_SINGLE_ARG) && method.getNumberOfParameters() == 1) || method.getNumberOfParameters() == 0;

Rather the opposite:

boolean showNamesOfSingleArguments= PreferenceConstants.getPreferenceStore().getBoolean(PreferenceConstants.EDITOR_JAVA_CODEMINING_SHOW_PARAMETER_NAME_SINGLE_ARG); return !showNamesOfSingleArguments && method.getNumberOfParameters() == 1 || method.getNumberOfParameters() == 0;

if you're into renaming with opposite meanings, I suggest renaming (and reversing conditions) from skipParameterNamesCodeMinings to showParameterNamesCodeMinings() { return number > 1 || PreferenceConstants.getPreferenceStore().getBoolean(PreferencesConstants.EDITOR_JAVA_CODEMINING_SHOW_PARAMETER_NAME_SINGLE_ARG)). That should make things simpler to understand and maintain.

No need to rename the method in this PR, it is fine as it is and it is overloaded so the renaming would have to be done for these 4 methods just so the class stays consistent:

mickaelistria · 2024-12-10T13:21:05Z

Thanks. That proposal is good to me!

szarnekow · 2024-12-10T13:26:47Z

...i/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/CalleeJavaMethodParameterVisitor.java

@@ -165,7 +165,13 @@ private boolean skipParameterNameCodeMining(String[] parameterNames, List<?> arg
 	}

 	private boolean skipParameterNamesCodeMinings(IMethod method) {
+		IPreferenceStore store= JavaPlugin.getDefault().getPreferenceStore();
+		boolean showFromOneParameter = store.getBoolean(PreferenceConstants.EDITOR_JAVA_SHOW_ONE_PARAMETER);


Do you really want to read from the store for each method?

And in line 179 another preference store access?

Would it be ok to leave that to another PR so that this one doesn't blow up?

should be displayed when using the code minings

fedejeanne · 2024-12-10T14:13:33Z

...dt.ui/ui/org/eclipse/jdt/internal/ui/preferences/JavaEditorCodeMiningConfigurationBlock.java

@@ -227,6 +230,9 @@ private void createGeneralSection(int nColumns, Composite parent) {
 				PreferencesMessages.JavaEditorCodeMiningConfigurationBlock_defaultFilterForParameterNames_label,
 				PREF_DEFAULT_FILTER_FOR_PARAMETER_NAMES, TRUE_FALSE, extraIndent, section);

+		fFilteredPrefTree.addCheckBox(inner,
+				"Show one Parameter", //$NON-NLS-1$


I'd suggest something more descriptive, like:

Suggested change

"Show one Parameter", //$NON-NLS-1$

"Show parameter names in methods with only 1 parameter", //$NON-NLS-1$

fedejeanne · 2024-12-11T07:37:19Z

...i/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/CalleeJavaMethodParameterVisitor.java

@@ -165,7 +165,7 @@ private boolean skipParameterNameCodeMining(String[] parameterNames, List<?> arg
 	}

 	private boolean skipParameterNamesCodeMinings(IMethod method) {
-		return method.getNumberOfParameters() <= 1;
+		return false;


No need to rename the method in this PR, it is fine as it is and it is overloaded so the renaming would have to be done for these 4 methods just so the class stays consistent:

fedejeanne · 2024-12-11T07:56:31Z

...i/ui/org/eclipse/jdt/internal/ui/javaeditor/codemining/CalleeJavaMethodParameterVisitor.java

@@ -165,7 +165,13 @@ private boolean skipParameterNameCodeMining(String[] parameterNames, List<?> arg
 	}

 	private boolean skipParameterNamesCodeMinings(IMethod method) {
+		IPreferenceStore store= JavaPlugin.getDefault().getPreferenceStore();
+		boolean showFromOneParameter = store.getBoolean(PreferenceConstants.EDITOR_JAVA_SHOW_ONE_PARAMETER);


Would it be ok to leave that to another PR so that this one doesn't blow up?

szarnekow · 2024-12-11T08:07:00Z

Would it be ok to leave that to another PR so that this one doesn't blow up?

Yes, sure.

jannisCode force-pushed the Code_minings_changes branch from cba8100 to 22a305c Compare November 26, 2024 14:10

jannisCode marked this pull request as ready for review November 26, 2024 14:11

fedejeanne reviewed Nov 27, 2024

View reviewed changes

jannisCode changed the title ~~The Code works now for every method~~ Provide name for parameters in 1-arg methods when using code minings Nov 29, 2024

jannisCode force-pushed the Code_minings_changes branch from a4a86ae to 7bc4aef Compare November 29, 2024 10:41

mickaelistria suggested changes Nov 29, 2024

View reviewed changes

jannisCode mentioned this pull request Dec 2, 2024

Record classes are now handled the same as normal classes #1822

Merged

3 tasks

jannisCode force-pushed the Code_minings_changes branch from 7bc4aef to 3462610 Compare December 10, 2024 12:23

mickaelistria reviewed Dec 10, 2024

View reviewed changes

jannisCode force-pushed the Code_minings_changes branch from 3462610 to 49f2c2d Compare December 10, 2024 13:14

szarnekow reviewed Dec 10, 2024

View reviewed changes

jannisCode force-pushed the Code_minings_changes branch 5 times, most recently from 9472b4d to 8dab19f Compare December 10, 2024 14:42

New Option in the preferences where one can change if one parameter

e301be1

should be displayed when using the code minings

jannisCode force-pushed the Code_minings_changes branch from 8dab19f to e301be1 Compare December 10, 2024 14:42

fedejeanne reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide name for parameters in 1-arg methods when using code minings #1813

Provide name for parameters in 1-arg methods when using code minings #1813

jannisCode commented Nov 26, 2024 •

edited

Loading

eclipse-jdt-bot commented Nov 26, 2024

mickaelistria commented Nov 26, 2024

fedejeanne commented Nov 27, 2024 •

edited

Loading

fedejeanne Nov 27, 2024

fedejeanne Dec 10, 2024

mickaelistria commented Nov 27, 2024

mickaelistria left a comment

fedejeanne commented Nov 29, 2024 •

edited

Loading

mickaelistria commented Nov 29, 2024

mickaelistria Dec 10, 2024

fedejeanne Dec 10, 2024

mickaelistria Dec 10, 2024

fedejeanne Dec 11, 2024

mickaelistria commented Dec 10, 2024

szarnekow Dec 10, 2024

szarnekow Dec 10, 2024

fedejeanne Dec 11, 2024

szarnekow Dec 11, 2024

fedejeanne Dec 10, 2024

fedejeanne Dec 11, 2024

fedejeanne Dec 11, 2024

szarnekow commented Dec 11, 2024

		@@ -165,7 +165,7 @@ private boolean skipParameterNameCodeMining(String[] parameterNames, List<?> arg
		}

		private boolean skipParameterNamesCodeMinings(IMethod method) {

	return false;
	return PreferenceConstants.getPreferenceStore().getBoolean(PreferencesConstants.EDITOR_JAVA_CODEMINING_SHOW_PARAMETER_NAME_SINGLE_ARG) && method.getNumberOfParameters() == 1) \|\| method.getNumberOfParameters() == 0;

	"Show one Parameter", //$NON-NLS-1$
	"Show parameter names in methods with only 1 parameter", //$NON-NLS-1$

Provide name for parameters in 1-arg methods when using code minings #1813

Are you sure you want to change the base?

Provide name for parameters in 1-arg methods when using code minings #1813

Conversation

jannisCode commented Nov 26, 2024 • edited Loading

What it does

Before:

After this change

Author checklist

eclipse-jdt-bot commented Nov 26, 2024

mickaelistria commented Nov 26, 2024

fedejeanne commented Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelistria commented Nov 27, 2024

mickaelistria left a comment

Choose a reason for hiding this comment

fedejeanne commented Nov 29, 2024 • edited Loading

mickaelistria commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelistria commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szarnekow commented Dec 11, 2024

jannisCode commented Nov 26, 2024 •

edited

Loading

fedejeanne commented Nov 27, 2024 •

edited

Loading

fedejeanne commented Nov 29, 2024 •

edited

Loading