homberghp commented on PR #8439: URL: https://github.com/apache/netbeans/pull/8439#issuecomment-2822438973
If the nb-java is patched, please add a getter for value, so that the FlagsMap can be made on the fly and go into e.g. nb-junit. > Before going into a review in more detail, a few high-level comments: > > * the patch seems to contain considerable amount of "debris" - commented out debugging code (sometimes not commented out debugging code), commented out code. Preferably, this should be cleaned. **Removed**. > * the patch has considerable amount of "drive-by" changes - changes that are not directly related to the purpose of the patch, but appear to improve the code. While some degree of that is OK, and while that may seem desirable, it complicates the review, and not always brings the value. In the patch, besides massive whitespace changes (which can, at least, be disabled while viewing the patch), there are changes like `VeryPretty.visitTopLevel`, where it is unclear what's the purpose of the changes. I would prefer if the drive-by changes would be reduced/minimized. **Removed the changes in VeryPretty. This happened quite early.** I will refrain from drive-ny changes that serve no other purpose than cleaner code. ( I still do not understand why the internals of an unsupported class are used, even when that class and implements java.util.List AND the classes doc states that is not meant to be used and may change over time.) As for the whitespace. That is what you get graits by NetBeans itself. I have discussed this with Michael Bien. The NetBeans developer should decide on a code style (google java style is advisable), make that the default for NetBeans or at least the default for the NetBeans project. At places the comprehensibility of the code suffers due to wrong indentation (and quite a few overly long lines). Not all developers can afford 4k monitors. > * public classes under `org.netbeans.api.java.source` (and other packages) are public API. This patch adds two: > > * `FlagsMap`: sorry, but surely not for the API, and probably not very good match at all. This class would require updates on ever javac upgrade, and also has some other problems (like that the flag numbers mean different things for different kinds of Symbols - for better or worse). If absolutely needed, there's `Flags.toString(long)`, which suffers from some issues as well (the ambiguity in number meanings), but at least does not require updates each time javac is upgraded. **removed** > * `RecordUtils`: while some API to work with records may be in order, introducing a new API class for them seems excessive. Classes like `TreeUtilities`/`ElementUtilities`/`TypeUtilities`, and, in some rare cases `SourceUtils` would be preferred. Also please note that JCTree is _not_ an API, and should not be used in API signatures. (I would also try to limit the newly added API entry points - it is possible to add new API entry points if needed, but it is difficult to remove them.) **removed** > * please note that new API entry points need `@since`, and an entry in `<module-name>/apichanges.xml`. **Not relevant anymore.** I hope the addition to nbjunit, AssertLinesEqualsHelper can stay. I would not know another place for it. -- 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
