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

Add support for message tags #105

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

Conversation

Half-Shot
Copy link

This enables the bridge to start parsing out messages tags from compliant IRC servers as per https://ircv3.net/specs/extensions/message-tags. It for the moment is very unspecified so all tags are returned as a Map to the client, to then use as it sees fit.

@Half-Shot Half-Shot requested a review from a team as a code owner May 9, 2023 16:57
@progval
Copy link

progval commented May 9, 2023

What's the reason for the supportsMessageTags option? Why not unconditionally allow message tags?
Also note that you may get message tags if you negotiate the account-tag or server-time capabilities, even without the message-tags capability (because they both predate the message-tags spec)

This is missing support for unescaping tag values.

for the moment is very unspecified so all tags are returned as a Map to the client, to then use as it sees fit.

that's the right way to do it in a library IMO

@Half-Shot Half-Shot force-pushed the hs/message-tag-support branch from 69fbe81 to d524393 Compare May 15, 2023 12:59
@Half-Shot
Copy link
Author

Half-Shot commented May 15, 2023

Escaping should now be working & tested.

As far as allowing support, it felt pretty sensible to check for the serverside support before parsing to ensure they support the exact spec we've been working against. Can probably also enable it with account tag or server time caps. I think any sever that wants to offer support for message tags should be broadcasting this though.

@progval
Copy link

progval commented May 15, 2023

s/escape/unescape/ in your last commit message, and you might want to add a test for that.

And having @tags but no :prefix is valid, it shouldn't raise "No prefix on message" (and the regexp should be updated to support it)

The rest looks good to me (though not tested myself)

@Half-Shot
Copy link
Author

s/escape/unescape/ in your last commit message, and you might want to add a test for that.

And having @tags but no :prefix is valid, it shouldn't raise "No prefix on message" (and the regexp should be updated to support it)

Failing to spot that in the spec for server sent messages, is there a reference somewhere?

@progval
Copy link

progval commented May 15, 2023

Servers MAY include a source on any message, and MAY leave a source off of any message. Clients MUST be able to process any given message the same way whether it contains a source or does not contain one.

-- https://modern.ircdocs.horse/#source

@Half-Shot
Copy link
Author

Started working on support for prefix-less messages, but didn't fully get there. Will come back to this.

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