vieiro commented on PR #5567: URL: https://github.com/apache/netbeans/pull/5567#issuecomment-1445464789
Thanks for your review, @matthiasblaesing . > I did a first pass and these are my notes: > > * All: The module names should not contain the folder name prefix `rust.XXXX -`, just the part after the "dash" I prefer having folder names over modules names, but no worries. Will do in next commits. > > * All: The commits should be squashed at least partially (19 seems to be to much) I'd squash squash all commits, I think. Will do in coming days. > > * Grammar: The *.interp and *.tokens files from org.netbeans.modules.rust.grammar.antlr4 package can be dropped. Then the licenseinfo.xml passage can be dropped as the java files already contain the ASF header. > Great. I think "*.tokens" is read when generating the grammar, but it's ok to leave it out of the repository. > * Grammar: Is it a good idea to modify the grammers (RustLexer.g4 and RustParser.g4) directly? For the CSS parser direct modifications were done and the grammar became disconnected from its origin, without an option to ever get updates from the source. This is ok, we just should now, whether we want to go down that road. I think RustLexer.g4 and RustParser.g4 must be included. These files are probably going to change frequently in the future, and we want everybody to contribute to them. This includes the original authors: the Rust Team. > > * Grammar: The build script rebuilds the java code from the grammar, yet the generated code is checked in. In other cases (golang, CSS) we generate the java code once and there are instructions in the generated files, that changes have to be made in the grammar files and rebuilding the java files is necessary. > We may or may not include the generated code. The "rust.grammar/build.xml" is responsible for updating the generated code before compilation whenever the `.g4` files are modified. > * Grammar: the Icons in org.netbeans.modules.rust.grammar.structure.resources need entries in the licenseinfo.xml > Very true. I ran `verify-libs-and-licenses` and no problems where detected, though. Thanks for pinpointing them. > * Project: It is surprising, that the "files" view shows less, than the "projects" view. All other types I'm aware of show a subset of data in the projects view and the files view gives access to data filterer in the project view > Will verify this. Any file missing in particular? > * Project: The ZIP file should not be there. There are two issues with this: we should not add ZIP files to two repository as the contents can be checked in directly. This then also allows to use the input in the wizard to update the `Cargo.toml` accordingly. The .gitignore file can be trivially created on the fly. +1 to that. Will modify the wizard. > > * Project API: src/org/netbeans/modules/rust/project/api/rust-logo.png is not used. +1 > > * Sources: The file `src/org/netbeans/modules/rust/sources/rs/templates/rust-file.png` should use `COMMENT_UNSUPPORTED` +1 Thanks for the thorough review, Matthias. Expect some changes during next week as time permits. -- 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
