-
Notifications
You must be signed in to change notification settings - Fork 147
[#1668] API types to simplify work with launch configuration attributes #1669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This pull request changes some projects for the first time in this development cycle.
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
Further information are available in Common Build Issues - Missing version increments. |
Test Results 1 947 files 1 947 suites 1h 37m 35s ⏱️ Results for commit e1b3b60. ♻️ This comment has been updated with latest results. |
As mentioned in the call, some example using this API proposal would be helpful. There is probably some content in org.eclipse.ui.externaltools/External Tools Base/org/eclipse/ui/externaltools/internal/launchConfigurations you can tweak to leverage this API. |
ceac906
to
0eaee71
Compare
I'm not sure why it is is failing @HannesWell
|
It looks like your change is breaking API-tools^^ |
Thank you @HannesWell for your assistance.
WOW! But that wasn't part of my plan. It looks like this issue was already reported. Interesting, may be I can have a deeper look to it soon. And still interested in your feedback regarding PR content. |
After supporting |
ok, a few more days for discussion and I'm going to merge it |
Thank you for that fix/enhancement of API-tools! It's much appreciated.
Sorry for not responding. It has been some busy days recently. I have not forgotten this, just didn't have the time for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a more detailed look at it and understand the goal and see the benefits from this: Better type-safety and more convenient read and writes.
Nevertheless I'm not sure that these benefits really carry the weight of the change, because currently the new API surface is very big.
Below I made some suggestion to make the API more compact, maybe we can then achieve a sufficiently got relation of advantages to API surface.
With these suggestions we would end up with just the LaunchAttributeIdentity
and LaunchAttributeDefined
types. Maybe we could even 'inline' the former into the latter and would then still get benefits mentioned above.
And eventually just having an ILaunchAttribute
interface added with a few static factories to create them is something that could indeed be a in my opinion suitable addition.
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LauchAttributeIdentityRecord.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeDefined.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeProbe.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeRead.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/LaunchAttributeWrite.java
Outdated
Show resolved
Hide resolved
...c/org/eclipse/core/externaltools/internal/launchConfigurations/LaunchAttributeArguments.java
Outdated
Show resolved
Hide resolved
if (String.class.equals(type)) { | ||
return type.cast(configuration.getAttribute(id, String.class.cast(value.get()))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we have this if-chain in the constructor and then encapsulate the action into a lambda and just execute it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not so sure
aebee8c
to
a278c9a
Compare
Thank you for your feedback @HannesWell PR was reimplemented according to your comments. |
I don't consider it as a fight, more as a search for (ideally) the best solution. Of course what is best is also a matter of opinion, but at least my personal opinion is that in general I like more compact APIs better. However can you please explain to me what the advantage is of defining in types what could be methods respectively why exactly this is Eclipse-2.0 style and why it's bad (I haven't been around back then)? But of course others can have other opinions about it and I invite everyone to share theirs. |
You are right @HannesWell , it's definitely a matter of personal opinion. For me the old good Eclipse 2.0 style means a lot of static definitions, favoring of primitive types in API and a lot of setters to reuse one instance as long as one can. All this was actual 25 years ago for ancient JVMs. Nowadays we have modern JVM and very dynamic environment where non-replaceable static definition can cause problems and saving memory due to mutability of objects does not justify the cost of (much more complex) code support. And even if I've stopped naming interfaces with "I" for new code and prefer simple immutable objects with very simple interfaces, I've done a lot following your comments to make this API improvement better suited to the surrounding code. Now the question is: what else should be improved in this PR? I still can see your "change requested" veto but your latest comment doesn't content any direct change requests. Please clarify. |
Hello @HannesWell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right @HannesWell , it's definitely a matter of personal opinion. For me the old good Eclipse 2.0 style means a lot of static definitions, favoring of primitive types in API and a lot of setters to reuse one instance as long as one can. All this was actual 25 years ago for ancient JVMs. Nowadays we have modern JVM and very dynamic environment where non-replaceable static definition can cause problems and saving memory due to mutability of objects does not justify the cost of (much more complex) code support.
My concern is not about a few objects more or less, I look at these new types from a consumer-perspective and how difficult a new API is to understand and learn. And there my experience/opinion is that 'distributed' types are more difficult and having it compact makes the capabilities easier to discover (as an IMO bad example: try to configure a JFace resource manager properly to be disposed with a Control and a child of the global registry).
As far as I know the problematic part of the Eclipse 2.0 style you referring to is that there tend to be only one large class that provided almost everything as static methods that don't allow dynamics, e.g. the Platform
class in eclipse.runtime.
But at least from my perspective we are here talking about static factories on types (classes or interfaces), which I don't consider old-style. In fact I have the impression that's often the preferred way over constructors and many are added in Eclipse. From the top of my head I remember e.g. ILog
or IPath
or in the JDK there is Path.of()
or the brand-new Classfile-API also makes heavy use of static factory methods AFAIK.
And even if I've stopped naming interfaces with "I" for new code and prefer simple immutable objects with very simple interfaces
Personally I'm also not fully convinced that it's best to prefix all interfaces I
. But in the end Eclipse is an existing code-base and when adding elements considering how new code fits into the existing code-base is IMO also an item to consider. Of course that doesn't make it simpler. Immutability is also something I think has many advantages and I try to achieve. For simple interfaces on the other hand, I would say it depends and that's IMO where the work starts and were the quality of an API is measured, if the 'right' size is found.
Now the question is: what else should be improved in this PR? I still can see your "change requested" veto but your latest comment doesn't content any direct change requests. Please clarify.
Sorry, I simply oversaw that you made changes. I was very busy of getting rid of the JXPath dependency in platform.ui.
I have now made another pass, please see the comments below.
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttribute.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttribute.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttribute.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttribute.java
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttribute.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttribute.java
Outdated
Show resolved
Hide resolved
...e.core.externaltools/src/org/eclipse/core/externaltools/internal/IExternalToolConstants.java
Outdated
Show resolved
Hide resolved
...e.core.externaltools/src/org/eclipse/core/externaltools/internal/IExternalToolConstants.java
Outdated
Show resolved
Hide resolved
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttributeIdentity.java
Outdated
Show resolved
Hide resolved
1d54c43
to
502eedc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update.
I didn't had the time for a detailed review. I still think we should handle the case where the default value depends on other attributes, because IIRC we have this case multiple times in PDE and I would like to have this covered. But I'll provide a specific example for that.
I'll be on vacation now, but when back I'll come back to this. Sorry for the further delay.
debug/org.eclipse.debug.core/core/org/eclipse/debug/core/ILaunchAttribute.java
Outdated
Show resolved
Hide resolved
146cae3
to
0d99094
Compare
1c2827e
to
425af03
Compare
@HannesWell I changed it to a state that we discussed on a meeting:
|
cfe45ed
to
e808a43
Compare
@HannesWell do you have more comments on this one? |
I didn't had the time yet to look into this, but definitely want to. |
* identify launch attribute * connect it with preference metadata (to supply defaults/label/description) * read attribute from configuration * write attribute to configuration working copy * demonstrate usage for ExternalTools
I only had the time for a short look at this change and in general I think this looks very good now. But I'd like to do an in depth review and try my best to do it tomorrow or Wednesday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update.
Conceptually I'm really happy with it now and for the API I just have one suggestion about the name of the probe method. And we should consider to have more factories to avoid verbosity in simple cases.
Everything else are just minor internal implementation details.
catch (CoreException e) { | ||
// do nothing | ||
} | ||
fCurrentLocation = IExternalToolConstants.LAUNCH_ATTRIBUTE_LOCATION.probe(configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before fCurrentLocation
was left unchanged, if getAttribute()
threw. But as the code below also handles empty optional, I think this change is fine.
*/ | ||
String UI_PLUGIN_ID = "org.eclipse.ui.externaltools"; //$NON-NLS-1$; | ||
String UI_PLUGIN_ID = "org.eclipse.ui.externaltools"; //$NON-NLS-1$ ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please remove the unnecessary trailing semicolon here and below?
String UI_PLUGIN_ID = "org.eclipse.ui.externaltools"; //$NON-NLS-1$ ; | |
String UI_PLUGIN_ID = "org.eclipse.ui.externaltools"; //$NON-NLS-1$ |
|
||
public static String LaunchAttributeArguments_name; | ||
|
||
|
||
public static String LaunchAttributeLocation_name; | ||
|
||
|
||
public static String LaunchAttributeWorkingDirectory_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid excessive new lines.
public static String LaunchAttributeArguments_name; | |
public static String LaunchAttributeLocation_name; | |
public static String LaunchAttributeWorkingDirectory_name; | |
public static String LaunchAttributeArguments_name; | |
public static String LaunchAttributeLocation_name; | |
public static String LaunchAttributeWorkingDirectory_name; |
@@ -1,5 +1,5 @@ | |||
/******************************************************************************* | |||
* Copyright (c) 2000, 2013 IBM Corporation and others. | |||
* Copyright (c) 2000, 2025 IBM Corporation and others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot see any relevant change in this class, beside white-space changes.
If that's correct, please leave this class untouched.
* must not be <code>null</code> | ||
* @return the {@link Optional} with attribute value | ||
*/ | ||
Optional<V> probe(ILaunchConfiguration configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit that I found the method name probe
a bit unusual (and I'm not even sure it fit's perfectly here).
What about just naming this method to the ordinary name get()
? With read one could somehow expect that it could fail.
Optional<V> probe(ILaunchConfiguration configuration); | |
Optional<V> get(ILaunchConfiguration configuration); |
* @param type the value type of the attribute, must not be | ||
* <code>null</code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of ``null
I generally would just use
{@code null}`.
* @param type the value type of the attribute, must not be | |
* <code>null</code> | |
* @param type the type of the attribute value, must not be {@code null} |
* @param value the {@link Function} to calculate of default value of the | ||
* attribute from {@link ILaunchConfiguration}, must not be | ||
* <code>null</code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe name this parameter defaultValue
?
* @param value the {@link Function} to calculate of default value of the | |
* attribute from {@link ILaunchConfiguration}, must not be | |
* <code>null</code> | |
* @param defaultValue the {@link Function} to calculate the default value of the | |
* attribute for a given {@link ILaunchConfiguration}, must not be | |
* {@code null}. |
As it's mentioned very often that an attribute may not be null
, what about stating that once in the interface description and only mention the cases where null is allowed (I assume nowhere?)
static <V> ILaunchAttribute<V> of(String id, Class<V> type, Function<ILaunchConfiguration, V> value, String name) { | ||
return new ConsiderStringVariables<>(id, type, value, name, name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As many callers just specify lc -> null
as default value, what do you think about providing two more overloads for convenience for attributes that have a fixed default and no default:
of(String id, Class<V> type, V defaultValue, String name)
of(String id, Class<V> type, String name)
They can then just call the main of
method with a corresponding lambda.
if (String.class.equals(type)) { | ||
working.setAttribute(id, String.class.cast(value)); | ||
} else if (Integer.class.equals(type)) { | ||
working.setAttribute(id, Integer.class.cast(value).intValue()); | ||
} else if (Boolean.class.equals(type)) { | ||
working.setAttribute(id, Boolean.class.cast(value).booleanValue()); | ||
} else if (List.class.equals(type)) { | ||
working.setAttribute(id, List.class.cast(value)); | ||
} else if (Map.class.equals(type)) { | ||
working.setAttribute(id, Map.class.cast(value)); | ||
} else if (Set.class.equals(type)) { | ||
working.setAttribute(id, Set.class.cast(value)); | ||
} else { | ||
working.setAttribute(id, value); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the distinction for the actual type actually necessary? AFAICT all special methods delegate to the object accepting variant any ways?
if (String.class.equals(type)) { | |
working.setAttribute(id, String.class.cast(value)); | |
} else if (Integer.class.equals(type)) { | |
working.setAttribute(id, Integer.class.cast(value).intValue()); | |
} else if (Boolean.class.equals(type)) { | |
working.setAttribute(id, Boolean.class.cast(value).booleanValue()); | |
} else if (List.class.equals(type)) { | |
working.setAttribute(id, List.class.cast(value)); | |
} else if (Map.class.equals(type)) { | |
working.setAttribute(id, Map.class.cast(value)); | |
} else if (Set.class.equals(type)) { | |
working.setAttribute(id, Set.class.cast(value)); | |
} else { | |
working.setAttribute(id, value); | |
} | |
working.setAttribute(id, value); |
Similarly, although I haven't checked it in detail, the readInternal
method maybe can be simplified too?
* | ||
* @see IStringVariableManager | ||
*/ | ||
record ConsiderStringVariables<V>(String id, Class<V> type, Function<ILaunchConfiguration, V> defaults, String name, String description) implements ILaunchAttribute<V> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name and description seem unused atm.
record ConsiderStringVariables<V>(String id, Class<V> type, Function<ILaunchConfiguration, V> defaults, String name, String description) implements ILaunchAttribute<V> { | |
record ConsiderStringVariables<V>(String id, Class<V> type, Function<ILaunchConfiguration, V> defaultValue) implements ILaunchAttribute<V> { |
Uh oh!
There was an error while loading. Please reload this page.