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.

Sign up for GitHub

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

Jump to bottom

Adding analyzer for log-levels concept exercise #136

Merged
merged 8 commits into from
Feb 29, 2024
Merged

Adding analyzer for log-levels concept exercise #136

merged 8 commits into from
Feb 29, 2024

Conversation

manumafe98
Copy link
Contributor

@manumafe98 manumafe98 commented Feb 19, 2024 •
edited
Loading

closes #107

To do:

@manumafe98 manumafe98 added the x:size/medium Medium amount of work label Feb 19, 2024
@manumafe98 manumafe98 self-assigned this Feb 19, 2024
@manumafe98 manumafe98 requested a review from a team as a code owner February 19, 2024 20:15
sanderploegsma
sanderploegsma requested changes Feb 21, 2024
View reviewed changes
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's looking very neat, nice work so far!

src/main/java/analyzer/exercises/loglevels/AvoidUsingStringFormat.java Outdated Show resolved Hide resolved
src/main/java/analyzer/exercises/loglevels/LogLevelsAnalyzer.java Outdated Show resolved Hide resolved
src/main/java/analyzer/exercises/loglevels/UseSubstringMethod.java Outdated Show resolved Hide resolved
src/main/java/analyzer/exercises/loglevels/UseSubstringMethod.java Outdated Show resolved Hide resolved
src/main/java/analyzer/exercises/loglevels/LogLevelsAnalyzer.java Show resolved Hide resolved
src/test/java/analyzer/AnalyzerIntegrationTest.java Show resolved Hide resolved
src/main/java/analyzer/exercises/loglevels/LogLevelsAnalyzer.java Outdated Show resolved Hide resolved
manumafe98 and others added 4 commits February 21, 2024 10:25
@manumafe98 @sanderploegsma
Update src/main/java/analyzer/exercises/loglevels/LogLevelsAnalyzer.java
417ce30 
Co-authored-by: Sander Ploegsma <[email protected]>
@manumafe98
Apllying suggestions
01161d9 
Renaming comment of string format to prefer_string_concatenation
Adding another helper method callsMethod
Making comment about substrings general
Adding another tests case for when substring is not being used in both methods
Making the analyzer to return only the hardcode and substring method if those trigger, so we do not overload the student of comments
@manumafe98
Adding missing files
daa8d03
@manumafe98
Fixing smoke tests
cf8d131
@manumafe98
Copy link
Contributor Author

@sanderploegsma I've found that for hardcoded tests it was being used this general comment I should use this one? Or the one I created is ok?

@sanderploegsma
Copy link
Contributor

@sanderploegsma I've found that for hardcoded tests it was being used this general comment I should use this one? Or the one I created is ok?

Great job for noticing, I forgot about that. If there is a general comment already, please use that instead of adding a new one! The only exception would be if you want the comment copy to contain more information that perhaps is exercise-specific, but I don't think that's the case here.

@manumafe98
Applying suggestions
c707caa 
Using general comment for hardcoded tests
Updating prefer string concatenation comment to be a general one
@manumafe98
Adding missing files
4196108
@manumafe98
Copy link
Contributor Author

@sanderploegsma I was thinking if we should also make this one a general comment ReuseCode if we are being vague with the lasagna one we can apply it to this one as well

@sanderploegsma
Copy link
Contributor

@sanderploegsma I was thinking if we should also make this one a general comment ReuseCode if we are being vague with the lasagna one we can apply it to this one as well

If you have a good idea for it then by all means! I suggest taking a look at the website copy first to see how it all works out.

@manumafe98
Copy link
Contributor Author

If you have a good idea for it then by all means! I suggest taking a look at the website copy first to see how it all works out.

Great! mostly because I was thinking on future analyzers that could also use the reusecode comment, so for me it makes more sense to make it a general one!

This was referenced Feb 28, 2024
Merged
Merged
sanderploegsma
sanderploegsma approved these changes Feb 29, 2024
View reviewed changes
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.

Awesome work! :shipit:

@manumafe98 manumafe98 merged commit c392d59 into exercism:main Feb 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanderploegsma sanderploegsma sanderploegsma approved these changes

@ErikSchierboom ErikSchierboom ErikSchierboom approved these changes

Assignees

@manumafe98 manumafe98

Labels
x:size/medium Medium amount of work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

log-levels: implement analyzer
3 participants
@manumafe98 @sanderploegsma @ErikSchierboom