-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AST Functionality #248
Conversation
…ption Handling and Synchronisation + Implemented Path Tool + Refactored Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this was intended for review already, but the comments will probably help anyway.
The overall direction where this is going is good, and most of the API looks as well.
Just some use cases are still unclear to me, mostly this has to do with the different types.
I think some classes can be merged, and the code might get more compact, the types may even fit in a single class with nested static classes. This might also reduce the number of sub-packages to zero.
Please adjust the changes such that all tests and code style checks still work.
Don't be scared by the number of comments, I just noted everything that came to my mind and could be improved. I hope it helps to have all that feedback as early as possible.
src/main/java/de/tum/in/test/api/ast/data/UnwantedNodesInformation.java
Outdated
Show resolved
Hide resolved
...egration/testuser/subject/structural/astTestFiles/classes/no/ClassWithNoKindsOfClasses2.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/test/api/ast/type/ExceptionHandlingType.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/test/api/ast/type/ExceptionHandlingType.java
Outdated
Show resolved
Hide resolved
I realize now, that apart from
or some similar APIs. The config+builder has the advantage that they can be specified once and then used for every assertion. We can also consider if varargs is a useful addition to Please tell me what you think about the suggestions; it would be very interesting to hear of other use cases than Could you ask the users (exercise creators at TUM for EIST/PSE/PGdP/...) what would be helpful? Maybe @JohannesStoehr has some suggestions as well, he knows both Ares and many instructors fairly well. |
- Deleted redundant test classes - Deleted SynchronizationType.java, as it caused more problems with tests than benefit Open aspect: - Replacement of Path.properties by reusing the functionality in de.tum.in.test.api.structural.testutils.ClassNameScanner.findObservedClassesInProject()
src/test/java/de/tum/in/test/integration/testuser/ITASTTestUser.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/test/api/ast/asserting/UnwantedNodesAssert.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/test/api/ast/asserting/UnwantedNodesAssert.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/test/api/ast/type/ExceptionHandlingType.java
Outdated
Show resolved
Hide resolved
Sadly had no time to do a full review, but I hope it helps anyway. Huge Thanks to @Strohgelaender for reviewing! Much appreciated 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I had a more in-depth look at the code now and changed a few small things myself, have a look if you agree. I also added the @API
tags as far as I understand, check if they are correct.
I think the changes for the most part will get more detailed now, as the approach in general is good, I think. Only some questionable aspects remain:
- The defaults for the assertion could become a problem, maybe we could make them explicit.
- The assertion is created new every time we configure it, this differs from the default AssertJ behavior. Maybe we should match that, although I like immutable objects.
- The way the files are located, I think this is still too much additional usage weight if we already have the configuration resolving in the class name scanner.
- The requirements, what the user's aims are, are still unclear to me for many node type enum values.
src/main/java/de/tum/in/test/api/ast/type/ExceptionHandlingType.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/test/api/ast/asserting/UnwantedNodesAssert.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better and better, and I am now happy with almost everything, the only part that stands out is the assertion and how the paths are handled. If we sort out how to do that cleanly, we can merge it. If you like, we can also have a quick talk about it when I have time. I think extracting the source folder search from the ClassNameScanner would yield clean code without too much effort.
...gration/testuser/subject/structural/astTestFiles/classes/yes/ClassWithAnyKindsOfClasses.java
Outdated
Show resolved
Hide resolved
...testuser/subject/structural/astTestFiles/conditionals/no/ClassWithNoKindsOfConditionals.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/in/test/integration/testuser/AstAssertionUser.java
Outdated
Show resolved
Hide resolved
I also added some Javadoc to public API classes. The tests stayed mostly the same, only the dot before relative paths got removed.
Suggestion: Simplify AST-API and Unify Project Sources Search
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, thank you @MarkusPaulsen for repeatedly improving the code and being patient.
This should enable us to release version 1.12.0 after a few dependency updates (the open PRs).
First implementation of the AST-checker, which can search through Java-Files for unwanted: