Replaced all [equals() -> "==", HashSet -> EnumSet, HashMap -> EnumMap] calls for enum types

[tbw777]

These changes are more readable, faster and safer at compile and run-time. Also == is null-safe than equals.

[BradWalker]

You've changed the modular import of discrete classes into a "include the entire kitchen sink" method.

This is a problem. It breaks the jdeps(1) dependency mapping. In addition, it kills the modularity of the project.

For this reason, I have to give it a -1. Can you please fix changes like this and resubmit.

[errael]

equals() -> "=="

Wouldn't Objects.equals(thing1,thing2) be a better choice? It's forward compatible, consider if any object add an equals method. And it handles null object OK.

[tbw777]

I think == is more preffered becouse of no methods used and also because compile time type checking

I was scaned the project and no enums with equals exist. Actual regexp:
^[^\"')(;,\;,\\s*{}/](\s)enum\s+[^{}/\[\]%&?@!#$\^\*<>]+\s*((implements)\s*[^{}/\[\]%&?@!#$\^\*]+\s*,\s)*{(.|\n)*public\s+boolean\s+equals

Exclusions:

1. enterprise/payara.tooling/src/org/netbeans/modules/payara/tooling/data/PayaraVersion.java

  @Override
  public boolean equals(PayaraPlatformVersionAPI version) {
      throw new UnsupportedOperationException("Not supported yet.");| Templates.
  }

2. enterprise/glassfish.tooling/src/org/netbeans/modules/glassfish/tooling/data/GlassFishVersion.java

 public boolean equals(final GlassFishVersion version) {
     if (version == null) {
         return false;
     } else {
         return this.major == version.major
                 && this.minor == version.minor
                 && this.update == version.update
                 && this.build == version.build;
     }
 }

And as described in https://docs.oracle.com/javase/specs/jls/se19/html/jls-8.html#jls-8.9

8.9.1. Enum Constants
Because there is only one instance of each enum constant, it is permitted to use the == operator in place of the equals method when comparing two object references if it is known that at least one of them refers to an enum constant.

The equals method in Enum is a final method that merely invokes super.equals on its argument and returns the result, thus performing an identity comparison.

[errael]

In my earlier/first quick glance, I thought I saw a non-enum change from .equals to ==. I must have been hallucinating, I don't see it now. Sorry for the noise.

[tbw777]

In my earlier/first quick glance, I thought I saw a non-enum change from .equals to ==. I must have been hallucinating, I don't see it now. Sorry for the noise.

Any feedback are welcome

[errael]

tbw777 requested review from errael

@tbw777 I'm not a NetBeans committer, I looked through it, looks OK. I think you're better off getting someone part of the NetBeans Organization for review.

[errael]

Looks OK.

I did not consider, from #5309 (review), the

You've changed the modular import of discrete classes into a

Which is probably most important.

[tbw777]

@errael
This comment was fixed after in new push. Click compare to review. Is problem actual or no? Ty

[BradWalker]

I removed my "do not merge" tag.. Thanks for fixing up the import statements.

[mbien]

looks good to me.

[mbien]

thanks for reviewing @BradWalker @errael, going to merge when all tests pass.

[mbien]

on second thought. We can't merge this in current form. Luckily the tests caught a problem.

EnumSet.copyOf(Collection) does not allow empty collections. All changes which use that method have to be checked again and/or reverted.

[tbw777]

@mbien reverted

[mbien]

please revert all changes to empty set and singleton sets. One is immutable the other is not.

you are now randomly changing already reviewed PRs.

[tbw777]

i will revert later

[errael]

please revert all changes to empty set and singleton sets. One is immutable the other is not.

@mbien @tbw777 I don't know git/github that well (I use mercurial) Is there a way to remove/revert the last change, rather than add/force another change on top of that? If another change is added on top of the last one, it seems difficult to know that all the empty/singleton changes are removed.

If the last change is removed, then the relatively small cleanup change could be added.

Side question. There seems to be at least two method to evolving PRs.

1. Multiple commits with squash when merged
2. Use force and keep it a single commit

Is 1. easier to work with (considering the current situation). They should give the same result, right?

Does NetBeans have guidance about this?

[mbien]

Side question. There seems to be at least two method to evolving PRs.

1. Multiple commits with squash when merged

we don't let gh squash third party PRs since gh does sometimes weird things with author information which depends on how the account is set up. PRs like this one here are merged as is - so the final commits are important and they are also checked via the paperwork job.

2. Use force and keep it a single commit

there is no rule. Not all PRs will be a single commit after merge. Some are better to be merged as multiple commits. Esp if the author does a bugfix + refactoring. Those have to be kept separate.

whatever makes it easier, there is no rule as long individual commits are buildable and green - otherwise this will cause trouble on git bisect.

This PR here was basically ready and only needed the fix for copyOf. I don't understand what is going on now tbh but I have no time to review it anyway atm. This will need a full review again.

(force pushes can be reviewed via the delta on github, so it is usually no problem as long the delta is small)

[neilcsmith-net]

Side question. There seems to be at least two method to evolving PRs.

1. Multiple commits with squash when merged
2. Use force and keep it a single commit

Is 1. easier to work with (considering the current situation). They should give the same result, right?

@errael side answer 😄 please don't squash and merge. Both GitHub UI and CLI have (different) negative side effects there. Do agree that multiple commits can be better when kept during reviews though. PR branches are generally writable by committers as well as authors, so squashing and force pushing into the PR branch (where warranted) for final checks before merging is ideal IMO.

[errael]

(force pushes can be reviewed via the delta on github, so it is usually no problem as long the delta is small)

Yeah, "Compare" is one of my favorite buttons.

To be clear: as evidenced by the compare buttons the changes made by the last forced push are known (at least to github). Is there a way to back up to a particular spot?

2d81672

was pretty clean

It had a few more changes than expected, but they were in line with the goal of this PR and looked OK. Only dealt with HashSet --> EnumSet

There were a small handful of items that drew my attention including a few things that got lost from the previous change.

[tbw777]

FIXED

[mbien]

looks like an import is missing.

   [repeat] /home/runner/work/netbeans/netbeans/enterprise/websvc.rest/src/org/netbeans/modules/websvc/rest/support/JavaSourceHelper.java:650: error: cannot find symbol
   [repeat]         modifierSet.addAll(Arrays.asList(modifiers));
   [repeat]                            ^

[tbw777]

Its because i havnt successful local builds since last days. I dont know what to do with a new message:

javafx-nbms:
[genkey] Generating Key for netbeans-bundled
[genkey] keytool error: java.security.KeyStoreException: Key protection algorithm not found: java.security.UnrecoverableKeyException: Encrypt Private Key failed: getSecretKey failed: Password is not ASCII
[genkey] Generating 2а048 bit RSA key pair and self-signed certificate (SHA256withRSA) with a validity of 90 days
[genkey] for: CN=Ant Group, OU=NetBeans, O=Apache.org, C=US
[nbmerge] Failed to build target: all-updatecenters

Excuse me
I was reviewed code again and there is no other red imports found.

[matthiasblaesing]

Its because i havnt successful local builds since last days. I dont know what to do with a new message:

javafx-nbms:
[genkey] Generating Key for netbeans-bundled
[genkey] keytool error: java.security.KeyStoreException: Key protection algorithm not found: java.security.UnrecoverableKeyException: Encrypt Private Key failed: getSecretKey failed: Password is not ASCII
[genkey] Generating 2а048 bit RSA key pair and self-signed certificate (SHA256withRSA) with a validity of 90 days
[genkey] for: CN=Ant Group, OU=NetBeans, O=Apache.org, C=US
[nbmerge] Failed to build target: all-updatecenters

Excuse me

I answered on the mailing list and you answered, that you did not change anything. You did not mention if you even tried to ensure, that the path you are building in only contains ascii characters.

There is no general problem in master, I build multiple time today from it and it works as always.

Please don't push PRs without at least ensuring that the changes actually work and the minimal validation for that is, that it compiles.

[mbien]

ran a build locally of this PR and it builds now. Enabling the tests again.

[tbw777]

@matthiasblaesing

I agree
You are right

The problem was in non ascii path. Now i can check all builds locally.

[errael]

Before #5309 (review) comment, I reviewed the PR and updates pretty carefully. After this comment, along with a little cleanup, the singleton/emptySet stuff happened. I checked this PR, ignoring the emptySet/singleton changes.

Then the change to get rid of the emptySet/singleton changes. I reviewed this change to insure that only singleton/emptySet stuff was changed. In addition to those changes, did find around 5 more changes for HashSet --> EnumSet, sigh. @tbw777, it's not unusual for there to be follow on PRs to clean up a few things, missed or otherwise, in a large PR as needed, rather than modifying a huge PR.

Now to verify that all the singleton/emptySet were reverted.

Pulled the PR, generated a diff, grep'd for "singleton" and "emptySet". Did not find any changes that included them (other than some white space).

So the commit looks OK to me. But I didn't start from scratch; another set of eyes...

(did find some unneeded changes, but I'm hesitant to point them out)

[mbien]

Looked through it again and it looks good to me. The revert caused some trailing whitespace artifacts but this shouldn't hold this off I think.

Tests are green too so I am planning to merge this tomorrow in case someone still wants to take a look.

[mbien]

Pulled the PR, generated a diff, grep'd for "singleton" and "emptySet". Did not find any changes that included them (other than some white space).

@errael here a trick (but don't tell anyone): just add .diff or .patch to get a diff/patch file
https://github.com/apache/netbeans/pull/5309.diff
https://github.com/apache/netbeans/pull/5309.patch

[errael]

a trick (but don't tell anyone): just add .diff or .patch to get a diff/patch file

No matter how hard I try, I keep learning things about github. Thanks ;-)

[errael]

@errael side answer smile please don't squash and merge. Both GitHub UI and CLI have (different) negative side effects there. Do agree that multiple commits can be better when kept during reviews though. PR branches are generally writable by committers as well as authors, so squashing and force pushing into the PR branch (where warranted) for final checks before merging is ideal IMO.

@neilcsmith-net I had always thought that references to CLI were about the git command. But I couldn't deny my ignorance anymore after reading what you said, yet another thing about git/github I didn't want to know.

So if I get this right, a pattern is to push several commits to a PR, then after approval, can squash locally (using mercurial in my case) and force push to the PR.

Does the compare button associated with a forced push after a local-squash/force-push show no changes (assuming, of course, that there were no changes)?

[neilcsmith-net]

@neilcsmith-net I had always thought that references to CLI were about the git command.

@errael in that context it did! Any local squash and merge pushed directly to master causes its own set of issues. Gone a bit off topic! Can follow that up on dev@ if more info needed.

resolved by PR author

[mbien]

no further comments, all green. merging. (removed old change request since it got resolved a while ago)