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

Fixing log levels analyzer #151

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Fixing log levels analyzer #151

merged 3 commits into from
Mar 27, 2024

Conversation

manumafe98
Copy link
Contributor

@manumafe98 manumafe98 commented Mar 18, 2024 •
edited by sanderploegsma
Loading

See #150

TODO:

@manumafe98
Fixing log levels analyzer
58ddbd8 
Updating substring comment to show on what method it should be applied
Udating hardcoded message check, so first converts the string to lowercase
@manumafe98 manumafe98 added the x:size/small Small amount of work label Mar 18, 2024
@manumafe98 manumafe98 self-assigned this Mar 18, 2024
@manumafe98 manumafe98 requested a review from a team as a code owner March 18, 2024 16:52
sanderploegsma
sanderploegsma approved these changes Mar 19, 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.

One minor detail, the rest LGTM!

src/main/java/analyzer/exercises/loglevels/UseSubstringMethod.java Outdated Show resolved Hide resolved
@manumafe98 manumafe98 mentioned this pull request Mar 19, 2024
Merged
@manumafe98
Copy link
Contributor Author

@sanderploegsma How we continue with this one after this comment

@sanderploegsma
Copy link
Contributor

Let's solve that in a separate PR. For now these changes at least clear up which method the feedback is addressing, which should prevent some confusion for students.

@manumafe98
Copy link
Contributor Author

Let's solve that in a separate PR. For now these changes at least clear up which method the feedback is addressing, which should prevent some confusion for students.

Sure, where we discuss it, in a new issue ?

@sanderploegsma
Copy link
Contributor

No need for a new issue, we can just use the existing issue to continue the conversation. Otherwise it would be harder to track.

I updated the PR description so that it no longer auto-closes the issue, so it can remain open until we're satisfied.

@manumafe98
Copy link
Contributor Author

No need for a new issue, we can just use the existing issue to continue the conversation. Otherwise it would be harder to track.

I updated the PR description so that it no longer auto-closes the issue, so it can remain open until we're satisfied.

Perfect!

@manumafe98
Copy link
Contributor Author

@ErikSchierboom Hey Erik! Can you please give us a hand with the review of this pr? Thanks!

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

@ErikSchierboom ErikSchierboom ErikSchierboom approved these changes

@sanderploegsma sanderploegsma sanderploegsma approved these changes

Assignees

@manumafe98 manumafe98

Labels
x:size/small Small amount of work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@manumafe98 @sanderploegsma @ErikSchierboom