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 metadata-2 spec #501

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

Add metadata-2 spec #501

wants to merge 59 commits into from

Conversation

progval
Copy link
Contributor

@progval progval commented Jun 25, 2022

As far as I am aware, it addresses all known issues with the current deprecated metadata spec (listed in #339).

It is based on @DanielOaks' latest version, which is itself based on #339. Notable changes since dan's version are:

  • standard replies (thanks to @ValwareIRC)
  • it's now a batch type instead of old-style RPL_ENDOFxxxx
  • capability is renamed to metadata-2, since it is a breaking change compared to the deprecated spec
  • metadata keys are now vendored to avoid clashes

rendered

@syzop
Copy link

syzop commented Jul 2, 2022

Indeed you said that and you are right, doing SUB in pre-registration is useful too because of auto join, otherwise the client has to send SYNC again (even for a small channel). Not devastating but still. And, indeed, perhaps at some point people would want to include it in channel history or there is some other use case for it that we cannot think of right now. Better to get it right.

Copy link

@Herringway Herringway left a comment

Choose a reason for hiding this comment

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

This wasn't tough to implement in my library, though there's a few things I think need addressing

extensions/metadata.md Outdated Show resolved Hide resolved
extensions/metadata.md Show resolved Hide resolved
extensions/metadata.md Show resolved Hide resolved
extensions/metadata.md Outdated Show resolved Hide resolved
extensions/metadata.md Outdated Show resolved Hide resolved
This subcommand returns which keys the client is currently subscribed to.

The server responds with zero or more `RPL_METADATASUBS` numerics. The server MAY return the keys in any order. The server MUST NOT list the same key multiple times in a response to this subcommand.

Choose a reason for hiding this comment

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

Without a batch or ending numeric, this is kinda tricky to deal with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Batches are a dependency of this spec

extensions/metadata.md Outdated Show resolved Hide resolved
ValwareIRC added a commit to ValwareIRC/ircv3-specifications that referenced this pull request Jul 4, 2022
- Corrected some keys in example displays
- Corrected a couple of missing ':' prefix in an example displays
- Deduplicated `RPL_METADATASUBS` in the numerics table
- Replaced RPL_ placeholders with their actual numeric value in examples

Only thing I didn't touch is [this one](ircv3#501 (comment)) purely because the spec already mentions that `metadata-2` requires `batch` to work, so shouldn't be a problem =]

Thank you Herringway for taking the time to point these out!
- Corrected some keys in example displays
- Corrected a couple of missing ':' prefix in an example displays
- Deduplicated `RPL_METADATASUBS` in the numerics table
- Replaced RPL_ placeholders with their actual numeric value in examples

Only thing I didn't touch is [this one](ircv3#501 (comment)) purely because the spec already mentions that `metadata-2` requires `batch` to work, so shouldn't be a problem =]

Thank you Herringway for taking the time to point these out!
extensions/metadata.md Outdated Show resolved Hide resolved
extensions/metadata.md Outdated Show resolved Hide resolved
@SadieCat
Copy link
Contributor

SadieCat commented Jul 7, 2022

Is there any reason for metadata-notify-2 to use a cap rather than an 005 token? From what I can see you can't subscribe before connection registration anyway, there's no need for immediate behaviour that needs to be performed on seeing it, and you now need to manually sub to any caps you want to see so it doesn't make sense for it to be a cap.

@Herringway
Copy link

Is there any reason for metadata-notify-2 to use a cap rather than an 005 token? From what I can see you can't subscribe before connection registration anyway, there's no need for immediate behaviour that needs to be performed on seeing it, and you now need to manually sub to any caps you want to see so it doesn't make sense for it to be a cap.

It's an opt-in feature. There's no standard way of opting into features advertised by ISUPPORT tokens.

I don't see the value in having it separate from metadata-2, though

@progval
Copy link
Contributor Author

progval commented Jul 8, 2022

METADATA now explicitly allowed in connection-registration, so the cap does make sense now

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

8 participants