Skip to content
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

Conversation

manumafe98
Copy link
Contributor

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.

@manumafe98 manumafe98 requested a review from a team as a code owner February 2, 2024 14:37
@manumafe98
Copy link
Contributor Author

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

@manumafe98
Copy link
Contributor Author

manumafe98 commented Feb 2, 2024

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.

@manumafe98 manumafe98 self-assigned this Feb 2, 2024
@manumafe98 manumafe98 added the x:size/medium Medium amount of work label Feb 2, 2024
@sanderploegsma
Copy link
Contributor

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

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.

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.

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:

  • Unit tests should exist to obtain as much code coverage as possible. Correctness of each exercise analyzer should be tested that way.
  • Each exercise should have at least two smoke tests: one with an optimal solution that receives no feedback, and one with a solution that receives at least one exercise-specific comment.
  • Next to that, a few smoke tests to cover exercises for which no analyzer is implemented should be present too, to make sure the analyzer works properly for every exercise. By that I mean that it shouldn't crash or something.

Once we come up with some concrete guidelines, we should probably write them down in the docs.

Copy link
Contributor

@sanderploegsma sanderploegsma left a 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.

src/main/java/analyzer/exercises/twofer/TwoferWalker.java Outdated Show resolved Hide resolved
tests/hamming/.meta/src/reference/java/Hamming.java Outdated Show resolved Hide resolved
Restoring MustUseStringCharAtOrCodePointAt analyzer functionality
Deleting unused usesConditional variable
Removing tests/hamming/.meta/src/reference/java/Hamming.java and tests/hamming/.meta/tests.toml
@manumafe98
Copy link
Contributor Author

Oh I think this is outdated as well:

private void validateNotNull(String leftStrand, String rightStrand) {

@sanderploegsma just in case, because it wasn't on your pervious review

Copy link
Contributor

@sanderploegsma sanderploegsma left a 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.

@sanderploegsma
Copy link
Contributor

@ErikSchierboom please review this one as well

@manumafe98 manumafe98 merged commit d7b8abe into exercism:main Feb 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:size/medium Medium amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants