You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2022/05/06 05:10:40 UTC
[GitHub] [netbeans] lbownik opened a new pull request, #4080: code and test cleaning
lbownik opened a new pull request, #4080:
URL: https://github.com/apache/netbeans/pull/4080
minor performance improvements
code upgrade to Java 8
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] matthiasblaesing commented on pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
matthiasblaesing commented on PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#issuecomment-1121547985
I think @vieiro tried to make the point, that this PR is to much noise for to little gain. Cosmetic changes by us nothing, but take time to review and see where breaks might have been introduced. PRs should fix problems, not cosmetics.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#discussion_r868370322
##########
platform/openide.util/src/org/openide/util/ChangeSupport.java:
##########
@@ -21,7 +21,7 @@
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
-import java.util.logging.Level;
+import static java.util.logging.Level.*;
Review Comment:
My reasoning is:
logger.log(FINE,..... is more compact than
logger.log(Level.FILE, .......
so it reads better (phrase "Level." is a noise here).
Since log method ony accepts Level objects as first parameter there is no doubt what "FINE" is and where it comes from (well .. absolute Java beginners might have doubts the first time ... but I don't think they contribute to NetBeans).
A counter example would be turning "Paths.get("C:\.....")" into "get(C:\...")". In this case uning static import would decrease readability.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik closed pull request #4080: code and test cleaning
Posted by "lbownik (via GitHub)" <gi...@apache.org>.
lbownik closed pull request #4080: code and test cleaning
URL: https://github.com/apache/netbeans/pull/4080
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] vieiro commented on pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
vieiro commented on PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#issuecomment-1121957029
> I think @vieiro tried to make the point, that this PR is to much noise for to little gain. Cosmetic changes by us nothing, but take time to review and see where breaks might have been introduced. PRs should fix problems, not cosmetics.
Exactly. Cosmetic changes add noise for little gain *and* introduce some risk in the implementation. NetBeans has >500k lines of code, and we use `import static`sparingly. Same applies for using the `this.` prefix, we use this sparingly in those other >500k files.
If you feel an urge to use `import static` for logging levels then it's better to have a single PR do this in 500k files than having 500k PRs changing each file.
Also modifying the implementation (of classes that are very sensitive to changes, since this module is probably used by everybody) and the tests at the same time is weird/risky. It's more difficult to know if the new code coming from somebody we don't know (or, as you say, _"knows too little"_) behaves the same as before. Until you gain experience I'd prefer PRs with tests only and PRs with code changes.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#discussion_r868372951
##########
platform/openide.util/src/org/openide/util/ChangeSupport.java:
##########
@@ -119,6 +108,7 @@ private void fireChange(ChangeEvent event) {
* false otherwise.
*/
public boolean hasListeners() {
- return !listeners.isEmpty();
+
+ return !this.listeners.isEmpty();
Review Comment:
ok
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on a diff in pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
lbownik commented on code in PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#discussion_r868372232
##########
platform/openide.util/src/org/openide/util/ChangeSupport.java:
##########
@@ -38,15 +38,16 @@ public final class ChangeSupport {
private static final Logger LOG = Logger.getLogger(ChangeSupport.class.getName());
// not private because used in unit tests
- final List<ChangeListener> listeners = new CopyOnWriteArrayList<ChangeListener>();
+ final List<ChangeListener> listeners = new CopyOnWriteArrayList<>();
private final Object source;
/**
* Creates a new <code>ChangeSupport</code>
*
* @param source the instance to be given as the source for events.
*/
- public ChangeSupport(Object source) {
+ public ChangeSupport(final Object source) {
Review Comment:
ok, I'm not religious about "final" - too bad "final" is not implicit in Java.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
lbownik commented on PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#issuecomment-1121591830
> I think @vieiro tried to make the point, that this PR is to much noise for to little gain."
I do understand the workload issue - I will not propose any solutions to this as I'm new to community and I know too little.
" Cosmetic changes by us nothing"
My experience tells me otherwise - often small cosmentic changes change the way I see the problem and help me find better solutions in the end. I can make small laser-focused PR's if this helps.
", but take time to review and see where breaks might have been introduced."
That's why we have unit tests. If this helps, I can intriduce separate PRs, one for tests cleaning and code cleaning.
I would submit code cleaning PR only after test cleaning PR gets merged.
"PRs should fix problems, not cosmetics."
I've seen this approach in action - it leads to code rot and spaghetti (and bancrupcy in case of commercial software).
Sustainable development is all about entropy, there needs to be constant conscious effort to reduce it, otherwise it kills eventually every project.
So would smaller PRs which tests first/code second approach work better?
I woud also skip final's and this'es.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] lbownik commented on pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
lbownik commented on PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#issuecomment-1121513373
"As far as I can see this is just cosmetic changes to support a new JDK version, without real improvements, right?"
Yes: The PR does not change the behavior.
No:
1. There are small improvements in readability (I can revert finals's and this'es if you insist).
2. There are small performance improvements:
2.1 I removed needles "ChangeSupport.fireChange(event)" method and merged it's body with the public "fireChange().
2.2 I relplaced iterators with lambdas in NBCollections.java (lambdas are 2x farster on my machine [Win 10 + Oracle JDK8]), which additionally shaved off 2 lines of code per change (readability).
2.3 I replaces 2 anonymous classes with lambdas in NBCollections.java (no need for class lodaer to load these any more).
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] vieiro commented on a diff in pull request #4080: code and test cleaning
Posted by GitBox <gi...@apache.org>.
vieiro commented on code in PR #4080:
URL: https://github.com/apache/netbeans/pull/4080#discussion_r867450893
##########
platform/openide.util/src/org/openide/util/NbCollections.java:
##########
@@ -34,9 +33,9 @@
import java.util.NoSuchElementException;
import java.util.RandomAccess;
import java.util.Set;
-import java.util.logging.Level;
+import static java.util.logging.Level.WARNING;
Review Comment:
We use `import static` sparingly. Not really requierd this change, right?
##########
platform/openide.util/src/org/openide/util/ChangeSupport.java:
##########
@@ -119,6 +108,7 @@ private void fireChange(ChangeEvent event) {
* false otherwise.
*/
public boolean hasListeners() {
- return !listeners.isEmpty();
+
+ return !this.listeners.isEmpty();
Review Comment:
`this` is unnecessary here, right? Why add it then?
##########
platform/openide.util/src/org/openide/util/ChangeSupport.java:
##########
@@ -21,7 +21,7 @@
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
-import java.util.logging.Level;
+import static java.util.logging.Level.*;
Review Comment:
This is a cosmetic unrequired change that makes code less legible and complicates review. Please revert it.
##########
platform/openide.util/src/org/openide/util/ChangeSupport.java:
##########
@@ -38,15 +38,16 @@ public final class ChangeSupport {
private static final Logger LOG = Logger.getLogger(ChangeSupport.class.getName());
// not private because used in unit tests
- final List<ChangeListener> listeners = new CopyOnWriteArrayList<ChangeListener>();
+ final List<ChangeListener> listeners = new CopyOnWriteArrayList<>();
private final Object source;
/**
* Creates a new <code>ChangeSupport</code>
*
* @param source the instance to be given as the source for events.
*/
- public ChangeSupport(Object source) {
+ public ChangeSupport(final Object source) {
Review Comment:
This is a cosmetic unrequired change that makes code less legible and complicates review. Please revert it.
##########
platform/openide.util/src/org/openide/util/NbCollections.java:
##########
@@ -34,9 +33,9 @@
import java.util.NoSuchElementException;
import java.util.RandomAccess;
import java.util.Set;
-import java.util.logging.Level;
+import static java.util.logging.Level.WARNING;
import java.util.logging.Logger;
-import org.openide.util.Enumerations;
+import static java.util.Objects.requireNonNull;
Review Comment:
Again another cosmetic change.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists