-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Remove redundant analyzer comments for hamming
and two-fer
#122
Remove redundant analyzer comments for hamming
and two-fer
#122
Conversation
…ttps://github.com/manumafe98/java-analyzer into RemoveRedundantAnalyzerCommentsForHammingAndTwoFer
I have some doubts regarding this issue, for example the print statements and main method comments on the two-fer expected_analysis.json are necessary? they are not mentioned in the design.md |
Also we should order the checks like the leap one for hamming and two fer? so we have some consistency? From what I've seen, is a folder for analyzer check. |
These comments are left by the global analyzer, so they are not exercise-specific. That is why they are not mentioned in the design.md.
Yes the smoke tests are a bit inconsistent, sorry for that. I think we should come up with some guidelines on how analyzers should be tested. My proposal would be:
Once we come up with some concrete guidelines, we should probably write them down in the docs. |
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.
Overall it looks good! I have left a couple of comments to look at but nothing major.
Restoring MustUseStringCharAtOrCodePointAt analyzer functionality Deleting unused usesConditional variable Removing tests/hamming/.meta/src/reference/java/Hamming.java and tests/hamming/.meta/tests.toml
Oh I think this is outdated as well:
@sanderploegsma just in case, because it wasn't on your pervious review |
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.
LGTM! I suggest you manually request a review from Erik to get the required admin approval, that way it's on his radar.
@ErikSchierboom please review this one as well |
Related issue: #96
The goal is to remove unnecessary comments, that will never trigger, because the analyzer only runs after the exercises passes the tests.