Refactoring: Simplify Profile
#5970
Conversation
enterprise/j2ee.core/src/org/netbeans/api/j2ee/core/Profile.java
Outdated
Show resolved
Hide resolved
enterprise/j2ee.core/src/org/netbeans/api/j2ee/core/Profile.java
Outdated
Show resolved
Hide resolved
|
to avoid the risk of someone adding an enum without updating the bundle properties we can inline the props into the class, consider removing Bundle.properties and applying this (includes changes from other comments): diff --git a/enterprise/j2ee.core/src/org/netbeans/api/j2ee/core/Profile.java b/enterprise/j2ee.core/src/org/netbeans/api/j2ee/core/Profile.java
index e16ae8b..f103a9f 100644
--- a/enterprise/j2ee.core/src/org/netbeans/api/j2ee/core/Profile.java
+++ b/enterprise/j2ee.core/src/org/netbeans/api/j2ee/core/Profile.java
@@ -24,6 +24,7 @@
import org.netbeans.api.annotations.common.NonNull;
import org.netbeans.api.annotations.common.NullAllowed;
import org.openide.util.NbBundle;
+import org.openide.util.NbBundle.Messages;
/**
* Represents the defined Java EE profiles.
@@ -34,48 +35,71 @@
// !!! ATTENTION: BE AWARE OF THE ENUM ORDER! It controls compatibility and UI position.
// Do not ever change name of this constant - it is copied from j2eeserver
+ @Messages("J2EE_13.displayName=J2EE 1.3")
J2EE_13("1.3"),
+
// Do not ever change name of this constant - it is copied from j2eeserver
+ @Messages("J2EE_14.displayName=J2EE 1.4")
J2EE_14("1.4"),
+
// Do not ever change name of this constant - it is copied from j2eeserver
+ @Messages("JAVA_EE_5.displayName=Java EE 5")
JAVA_EE_5("1.5"),
+
+ @Messages("JAVA_EE_6_WEB.displayName=Java EE 6 Web")
JAVA_EE_6_WEB("1.6", "web"),
+
+ @Messages("JAVA_EE_6_FULL.displayName=Java EE 6")
JAVA_EE_6_FULL("1.6"),
+
+ @Messages("JAVA_EE_7_WEB.displayName=Java EE 7 Web")
JAVA_EE_7_WEB("1.7", "web"),
+
+ @Messages("JAVA_EE_7_FULL.displayName=Java EE 7")
JAVA_EE_7_FULL("1.7"),
+
+ @Messages("JAVA_EE_8_WEB.displayName=Java EE 8 Web")
JAVA_EE_8_WEB("1.8", "web"),
+
+ @Messages("JAVA_EE_8_FULL.displayName=Java EE 8")
JAVA_EE_8_FULL("1.8"),
+
+ @Messages("JAKARTA_EE_8_WEB.displayName=Jakarta EE 8 Web")
JAKARTA_EE_8_WEB("8.0", "web"),
+
+ @Messages("JAKARTA_EE_8_FULL.displayName=Jakarta EE 8")
JAKARTA_EE_8_FULL("8.0"),
+
+ @Messages("JAKARTA_EE_9_WEB.displayName=Jakarta EE 9 Web")
JAKARTA_EE_9_WEB("9.0", "web"),
+
+ @Messages("JAKARTA_EE_9_FULL.displayName=Jakarta EE 9")
JAKARTA_EE_9_FULL("9.0"),
+
+ @Messages("JAKARTA_EE_9_1_WEB.displayName=Jakarta EE 9.1 Web")
JAKARTA_EE_9_1_WEB("9.1", "web"),
+
+ @Messages("JAKARTA_EE_9_1_FULL.displayName=Jakarta EE 9.1")
JAKARTA_EE_9_1_FULL("9.1"),
+
+ @Messages("JAKARTA_EE_10_WEB.displayName=Jakarta EE 10 Web")
JAKARTA_EE_10_WEB("10", "web"),
+
+ @NbBundle.Messages("JAKARTA_EE_10_FULL.displayName=Jakarta EE 10")
JAKARTA_EE_10_FULL("10");
// !!! ATTENTION: BE AWARE OF THE ENUM ORDER! It controls compatibility and UI position.
- public static final Comparator<Profile> UI_COMPARATOR = new Comparator<Profile>() {
-
- @Override
- public int compare(Profile o1, Profile o2) {
- return -(o1.ordinal() - o2.ordinal());
- }
- };
+ public static final Comparator<Profile> UI_COMPARATOR = (Profile o1, Profile o2) -> -(o1.ordinal() - o2.ordinal());
// cache
private final String propertiesString;
private Profile(String canonicalName) {
- this(canonicalName, null);
+ this.propertiesString = canonicalName;
}
private Profile(String canonicalName, String profile) {
- StringBuilder builder = new StringBuilder(canonicalName);
- if (profile != null) {
- builder.append("-").append(profile); // NOI18N
- }
- this.propertiesString = builder.toString();
+ this.propertiesString = canonicalName + "-" + profile;
}
/**
@@ -132,7 +156,7 @@
for (Profile profile : Profile.values()) {
if (profile.toPropertiesString().equals(valueMinusQuotes)
|| profile.name().equals(valueMinusQuotes)
- || profile.name().endsWith(valueMinusQuotes)) {
+ || (valueMinusQuotes.startsWith("EE_") && profile.name().endsWith(valueMinusQuotes))) {
return profile;
}
}
|
|
to fix the tests we have to make two more changes: have have to change and tests: diff --git a/enterprise/j2ee.core/test/unit/src/org/netbeans/api/j2ee/core/ProfileTest.java b/enterprise/j2ee.core/test/unit/src/org/netbeans/api/j2ee/core/ProfileTest.java
index 34bfc1f..6a1a67d 100644
--- a/enterprise/j2ee.core/test/unit/src/org/netbeans/api/j2ee/core/ProfileTest.java
+++ b/enterprise/j2ee.core/test/unit/src/org/netbeans/api/j2ee/core/ProfileTest.java
@@ -229,8 +229,8 @@
assertFalse(Profile.JAKARTA_EE_8_FULL.isAtLeast(Profile.JAKARTA_EE_10_WEB));
assertFalse(Profile.JAKARTA_EE_9_WEB.isAtLeast(Profile.JAKARTA_EE_10_WEB));
assertFalse(Profile.JAKARTA_EE_9_FULL.isAtLeast(Profile.JAKARTA_EE_10_WEB));
- assertTrue(Profile.JAKARTA_EE_9_1_WEB.isAtLeast(Profile.JAKARTA_EE_10_WEB));
- assertTrue(Profile.JAKARTA_EE_9_1_FULL.isAtLeast(Profile.JAKARTA_EE_10_WEB));
+ assertFalse(Profile.JAKARTA_EE_9_1_WEB.isAtLeast(Profile.JAKARTA_EE_10_WEB));
+ assertFalse(Profile.JAKARTA_EE_9_1_FULL.isAtLeast(Profile.JAKARTA_EE_10_WEB));
assertTrue(Profile.JAKARTA_EE_10_WEB.isAtLeast(Profile.JAKARTA_EE_10_WEB));
assertTrue(Profile.JAKARTA_EE_10_FULL.isAtLeast(Profile.JAKARTA_EE_10_WEB));
}after that we can add them to the - name: j2ee.core
run: ant $OPTS -f enterprise/j2ee.core test |
What do you think about having a test validating that the |
the annotations generate a bundle at build time. Having the default property values directly above the enum makes later updates which add more enum values straight forward since everything is in one place. I wouldn't go crazy with validation here since this PR makes future updates already much easier and therefore less error prone. |
Is that profile information stored somewhere? Are you sure there are no side effets when we change from |
no I am not sure, that is something I have to check before we merge. This would be a problem in any case since the tests already failed even without this PR applied (they expect |
mbien
added
the
ci:dev-build
Didn't realized you already responded :D Actually prior this PR I already experimented with moving out the UI stuff out of that class as from my understanding it's most likely used for backend related stuff. I just reverted it as it results into an API breakage and several other classes needed to be touched. From my experience mixing both can get you in messy situations - Maybe in this case it's a little bit too much, but I wonder if this class should contain any UI related stuff (like the |
enterprise/j2ee.core/test/unit/src/org/netbeans/api/j2ee/core/ProfileTest.java
Show resolved
Hide resolved
| // profileToCompare has lower version than comparingVersion | ||
| return comparisonResult <= 0; | ||
| for (Profile profile : Profile.values()) { | ||
| if (profile.toPropertiesString().equals(valueMinusQuotes) |
There was a problem hiding this comment.
@asbachb what we could do is to make it a "10.0" in the profile, but change this line to:
if (profile.toPropertiesString().equals(valueMinusQuotes) || profile.toPropertiesString().equals(valueMinusQuotes+".0"))This wouldn't break backwards compatibility while also not breaking consistency. What do you think?
There was a problem hiding this comment.
nevermind, this wouldn't work due to -web
There was a problem hiding this comment.
lets keep it as is. At some point in future we can do the following:
- remove the overridden
toString() - return the enum
name()intoPropertiesString()
This will get rid of the properties mess for new projects and simply use the enum name. The fromPropertiesString() method is already supporting it and is also able to parse all the old properties.
There was a problem hiding this comment.
@asbachb what we could do is to make it a
"10.0"in the profile, but change this line to:if (profile.toPropertiesString().equals(valueMinusQuotes) || profile.toPropertiesString().equals(valueMinusQuotes+".0"))This wouldn't break backwards compatibility while also not breaking consistency. What do you think?
I'd say if there's this is not causing any issues somewhere else we should live with the current implementation. I don't see the benefit in changing it when this means to do major refactoring, think about possible backward compatibilities or introduce some quirks.
asbachb
force-pushed
the
simplify-javaee
branch
2 times, most recently
from
7c018a1
to
fb47a15
Compare
|
Added |
|
@mbien Any open issues for now? I have some more ideas for simplifications. Should this get merged first or should I add the changes into this PR? |
mbien
removed
the
ci:dev-build
|
@asbachb could you squash the two commits? I can merge once tests are green if this PR is blocking something. |
This enables to iterate the enum values which simplifies the entire class/enum quite a lot: * New profiles just need to be added to enum * Order can be replaced by `ordinal` Enum order now is also used for profile comparison which eliminates a huge part of the class/enum.
During preparation of #5969 I recognized that it requires a lot of manual adjustments when a new Jakarta EE release is published. Several methods use long if/else statements which get longer with every new release.
One problem seems to be that it's not easily possible to iterate through the available
Profiles.In order to be able to iterate through the available
Profiles I propose to change it to an enum. This eliminates also things as the order and the need to set a specific bundle property name as it can be now generated from the enum name.Signature needs to be adjusted for sigtest, but from a callers perspective there is no api change when using that class/enum.