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

Improved emote parsing in PrivateMessage #1

Merged
merged 3 commits into from Sep 12, 2022

Conversation

kevinrpb
Copy link
Contributor

@kevinrpb kevinrpb commented Sep 9, 2022

Hi there!

I've been using your package for a personal project and it is great! Thank you very much for all the work. Since it has been very helpful, I thought I'd give back by improving something I found was not as straight forward as other aspects of the message.

Up to now, the parsing of the emotes in the message was a bit odd, specially for handling messages where the same emote repeats multiple times.

I made a small utility function that takes care of better parsing the information from the message into an array containing a new struct EmoteReference. This struct includes the id of the emote, the name (aka the text used to represent the emote), and the indices in the message that correspond to the emote. The idea is that having all this information it is easier for a user of the package to handle emotes.

Please, let me know if there's anything out of place or if I should include some more test cases (although I feel like the one included already handles all cases).

Cheers!

@MahdiBM MahdiBM self-requested a review September 9, 2022 21:15
Copy link
Owner

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.
It looks good but I would like some fixes/improvements.
I've left some comments.

}

/// Try and retrieve the parts and add them to our list of emotes
if let lastID,
Copy link
Owner

Choose a reason for hiding this comment

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

We cannot use this kind of variable binding because we need to support older swift versions than 5.7.
change this to let lastID = lastID,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! That's my bad, I could only run the tests on Xcode 14 :)

Sources/TwitchIRC/IncomingMessage/PrivateMessage.swift Outdated Show resolved Hide resolved
Sources/TwitchIRC/IncomingMessage/PrivateMessage.swift Outdated Show resolved Hide resolved
Sources/TwitchIRC/Utils/String Extensions.swift Outdated Show resolved Hide resolved
Sources/TwitchIRC/Utils/String Extensions.swift Outdated Show resolved Hide resolved
@kevinrpb
Copy link
Contributor Author

I made the changes, thank you for taking the time to review the PR.

However, I feel like Equatable conformance is at least nice since it allows automatic synthesis. Let me know if there are any downsides to this that I might be missing.

Cheers 😃

@MahdiBM
Copy link
Owner

MahdiBM commented Sep 12, 2022

Thanks for the changes, I'll take a look.

I do agree that the automatic synthesized conformance for Equatable is nice, but then if I want to allow it, one could argue "Why the other structs don't have the conformance?".
So its a question of if I want to go all in for the conformance or not.
I also like to keep the library minimal. I've been in situations that i would have preferred TwitchIRC's stuff be Codable so its easier for me to encode them, but it wouldn't make sense to add it to the library because its unrelated to the library and it was just my needs.

@MahdiBM
Copy link
Owner

MahdiBM commented Sep 12, 2022

Thanks again. This looks good.
I'm going to merge this and do an extra commit for some nit-picky cleanup.
You / I can utilize Emote in other files as well. For now this is enough and can act like a test to see if Emote works as expected.

@MahdiBM MahdiBM merged commit 589525b into MahdiBM:main Sep 12, 2022
@MahdiBM
Copy link
Owner

MahdiBM commented Sep 12, 2022

This will be tagged in a few days. This also contains a breaking change, so hopefully its worth it.

@kevinrpb
Copy link
Contributor Author

I do agree that the automatic synthesized conformance for Equatable is nice, but then if I want to allow it, one could argue "Why the other structs don't have the conformance?".

Yes, I can see that. It's really not a big problem, and "keeping it simple" is a good argument for it. If only the compiler could auto-synthesize from an extension 😄.

You / I can utilize Emote in other files as well. For now this is enough and can act like a test to see if Emote works as expected.

I've been using it in my own app with good success to load up the emotes. The only thing I am missing now is getting emote sets that are not global or the room's. I will look into that and perhaps send another PR.

This will be tagged in a few days. This also contains a breaking change, so hopefully its worth it.

Understood 👍🏽

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.

None yet

2 participants