mbien commented on PR #8799: URL: https://github.com/apache/netbeans/pull/8799#issuecomment-3274340052
just for fun: <details> As dev, if you see a key like [OK_OPTION](https://bits.netbeans.org/dev/javadoc/org-openide-dialogs/org/openide/NotifyDescriptor.html#OK_OPTION) in the doc with no other comments, you have two options: Use `equals()` and sleep well, knowing to be completely on the safe side no matter what the key value is. Or trust the library to do the right thing and use `==` while sleeping slightly less well. The "right thing" would be to not use distinct boxed instances as keys, when two keys share the same value but not instance. This gives mixed signals. I believe if the API would have exposed the keys as Integer, it would make it less likely that someone would use `==` since many would remember the java object identity riddles with boxed types and also Strings. Devs however who write code which does `if (choice.equals(JOptionPane.OK_OPTION)` (compares an `Object` with an `int` without any documented correlation between the two keys) or `showEncodingWarnings = (Integer) dd.getValue()` choose to live dangerously (and sleep less) relying on a impl detail. It is completely fine do do such things if your release process is prepared for quick updates, since you essentially set an impl dependency to `org-openide-dialogs`. I am not super worried about this category. By replacing `new` with `valueOf`, this PR ensures that the good dev who decided to write clean code with equals, can continue to sleep well. Incidentally, the dev who decided to live dangerously and sleep less, will still have running code. However, the dev who decided to risk a little bit more by hoping the dependency does the right thing and used '==', has the small potential that something might break. In contrast, a custom return box based on a record, might break the code of the dev who wanted to live dangerously, but the probability for that is pretty high. The clean code and slight-risk devs will be fine. (the solution is also somewhat less elegant and if someone in future puts 'value' in front of the record -> we are back on square one :)) </details> >> once Integer is a value based object, OK option will have the same == identity as the YES option, this PR is one way of addressing this issue before applications break. > Is this the only thing that would break? well the little key identity crisis would already break our impl, the tests did notice it too although indirectly (thats why this PR has to change impl code when OK is made identical to YES). JEP 401 is worth reading btw, well written including ASCII art. What if the constructors are gone too? Our build would fail and NB 27 based apps might potentially no longer run at all if run on newer JDKs since `NotifyDescriptor` would fail during static class initialization phase. There is now a chance that they stay though. `Set.of(NotifyDescriptor.YES_OPTION, NotifyDescriptor.OK_OPTION).contains(NotifyDescriptor.OK_OPTION)` will consistently throw exceptions with all proposals and on all timelines :) so I am +1 for Neil's proposal assuming nothing shows up during testing. But I think we should update the doc to mention that the key values are boxed `JOptionPane` keys to "become clean" API-wise. I would be curious if changing the keys from `Object` to `Integer` would be a compatible change? (I would have to read up on that - it might be) This would have a self-documenting effect. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information about the NetBeans mailing lists, visit: https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
