review: feat: add mode ignore-syntax-errors #4054
Conversation
andrewbwogi
changed the title
There was a problem hiding this comment.
Hi @andrewbwogi, thanks for putting in this work!
I'll leave it to @monperrus to provide final feedback on this PR, but I have some initial thoughts.
First of all, the name "filter invalid" is not precise to me, it doesn't immediately tell me the purpose of the option. The word "filter" is a bit ambiguous in its own right (although in functional programming it's a pretty ubiquitous function), and "invalid" does not specify what is invalid. How about something like "ignore syntax errors" or "ignore invalid syntax" instead? Or perhaps even "ignore files with invalid syntax", which precisely defines the functionality, but locks the semantics of this option to ignore entire files (perhaps we want to ignore parts of files in the future).
I also left a comment on the test method, it ought to be refactored.
| final TestLogger logger = TestLoggerFactory.getTestLogger(TreeBuilderCompiler.class); | ||
| Launcher launcher = new Launcher(); | ||
| launcher.getEnvironment().setFilterInvalid(true); | ||
| launcher.addInputResource("./src/test/resources/compilation2/InvalidClass.java"); | ||
| launcher.addInputResource("./src/test/resources/compilation/ClassWithStaticFields.java"); | ||
| launcher.buildModel(); | ||
| CtModel model = launcher.getModel(); | ||
| assertEquals(1,model.getAllTypes().size()); | ||
|
|
||
| // contract: if a file has any syntax errors, the incorrect file name is logged | ||
| assertTrue(logger.getLoggingEvents().get(0).getMessage().startsWith("Syntax error detected in")); | ||
|
|
||
| // contract: if every input resource has a syntax error, spoon does not crash | ||
| launcher = new Launcher(); | ||
| launcher.getEnvironment().setFilterInvalid(true); | ||
| launcher.addInputResource("./src/test/resources/compilation2/InvalidClass.java"); | ||
| launcher.buildModel(); | ||
| model = launcher.getModel(); | ||
| assertTrue(model.getAllTypes().isEmpty()); | ||
|
|
||
| // contract: filter-invalid can be enabled with a command line argument | ||
| launcher = new Launcher(); | ||
| launcher.setArgs(new String[]{"--filter-invalid", "-i", "./src/test/resources/compilation2/InvalidClass.java"}); | ||
| launcher.buildModel(); | ||
| model = launcher.getModel(); | ||
| assertTrue(model.getAllTypes().isEmpty()); |
There was a problem hiding this comment.
This is four different tests squeezed into one. Arguably, the logger test could be squeezed into the first one to save space, but the two tests at the bottom are completely independent of the rest of the method and should absolutely be tests of their own.
|
Thanks @andrewbwogi for this PR.
I like |
andrewbwogi
changed the title
There was a problem hiding this comment.
Nice @andrewbwogi, thanks!
I've no further comments, but as this is a new feature I'd like @monperrus to have the final look and merge.
Ping @monperrus :)
|
Perfect! Thanks a lot @andrewbwogi for the PR and @slarse for the review! |
closes #3954, closes #3952