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

Karma: Strip Spaces and characters appended to Nicks by clients #1496

Closed
wants to merge 2 commits into from

Conversation

prdes
Copy link
Contributor

@prdes prdes commented Nov 16, 2021

Attempt towards #1495

Issues:
Hardcoded inc/dec chars

@prdes
Copy link
Contributor Author

prdes commented Nov 16, 2021

I'm unconvinced and shall test on instance and write tests. How rlstrip() works that way!

@prdes prdes changed the title initial commit with hardcoded inc & dec chars with parenthesis support Karma: Strip Spaces and characters appended to Nicks by clients Nov 17, 2021
@prdes
Copy link
Contributor Author

prdes commented Nov 17, 2021

The squash and fixup didnt go perfect. The space, , and : chars are rstrip()'d from nicks

Added tests for the same

@prdes prdes marked this pull request as ready for review November 17, 2021 00:49
inc = self.registryValue('incrementChars', msg.channel, irc.network)
dec = self.registryValue('decrementChars', msg.channel, irc.network)
if msg.args[1].endswith(inc) or msg.args[1].endswith(dec):
thing = msg.args[1].rstrip(":, ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this will actually work - if the message ends with ++ or -- as in the default configuration, there is no way it'll also end with one of :, , so this is effectively a no-op.

I think this code should belong in either _doKarma or _normalizeThing so the behaviour is consistent with addressed Karma calls.

@prdes
Copy link
Contributor Author

prdes commented Nov 18, 2021

bad logic

@prdes prdes closed this Nov 18, 2021
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