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

Fixes cosmetic issue where the karma message target should match the … #278

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jottinger
Copy link
Collaborator

…requested target

i.e., "~karma FoO" should say "FoO has karma 0" and not the normalized "foo has karma 0"

…requested target

i.e., "~karma FoO" should say "FoO has karma 0" and not the normalized "foo has karma 0"
@jottinger jottinger requested a review from evanchooly May 19, 2020 11:46
@@ -88,10 +88,10 @@ class KarmaOperation @Inject constructor(bot: Javabot, adminDao: AdminDao, var d
}
increment = false
}
var karma: Karma? = dao.find(nick)
var karma: Karma? = dao.find(nick.toLowerCase())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this search is already case insensitive. it, in fact, upper cases the name before querying.

if (karma == null) {
karma = Karma()
karma.name = nick
karma.name = nick.toLowerCase()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also feels unnecessary

val nick = message.substring("karma ".length).toLowerCase()
val karma = dao.find(nick)
val nick = message.substring("karma ".length)
val normalizedNick=nick.toLowerCase()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto. the search already ignores case. lowercasing it is redundant. ( i know it was that way before but it was wrong then, too)

@evanchooly
Copy link
Owner

@dependabot recreate

1 similar comment
@evanchooly
Copy link
Owner

@dependabot recreate

@jottinger
Copy link
Collaborator Author

This has been updated, but I can't build it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants