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

Standard Replies Extension #357

Merged
merged 9 commits into from Aug 5, 2019
Merged

Standard Replies Extension #357

merged 9 commits into from Aug 5, 2019

Conversation

@DanielOaks
Copy link
Member

@DanielOaks DanielOaks commented Feb 4, 2019

We've been wanting to create this for a while, and for specs like #349 and #276 we want to actually start using it.

Basically, the FAIL message is intended to replace future numeric reservations for the purpose of conveying general error information. If you create a new error response, implements shouldn't need to reserve a new numeric, and instead are simply able to use a different error code and description.

Right now, an FAIL message is general – it can either mean a complete failure to execute the given command, or it can mean that there's simply something for the user/client software to keep in mind. This is consistent with how ERR_ numerics are used today. If we instead wish to present fatal (or very impacctful) and non-fatal/impactful errors separately, it may be worth also defining the WARN message which takes the same format as the FAIL message.

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 4, 2019

Considering a change of verb to FAIL, to very explicitly differentiate this from the existing ERROR message which indicates "OH NO CONNECTION IS DEAD" and would be veeery confusing. If consideration's go, will change up the spec and issue title appropriately.

@DanielOaks DanielOaks changed the title ERR Message FAIL Message Feb 4, 2019
extensions/fail.md Outdated Show resolved Hide resolved
extensions/fail.md Outdated Show resolved Hide resolved
extensions/fail.md Outdated Show resolved Hide resolved
@Alexendoo
Copy link

@Alexendoo Alexendoo commented Feb 5, 2019

Might it be worth defining a vendor-specific/standard distinction for error-codess? Much in the same way as message tags

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 6, 2019

I'm not exactly sure about vendor prefixing or something along those lines. May be a good idea to have a FAIL error-code registry on the v3 site – less a must-be-here-before-use and more of a here's the codes that software is using out there, try to use these. Added a sentence about converging on error codes used by other software in the meantime.

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 6, 2019

For what it's worth, new features that do explicitly send out FAIL messages will define the stock error codes that can be sent by their command as part of their specifications.

extensions/fail.md Outdated Show resolved Hide resolved
@prawnsalad
Copy link
Contributor

@prawnsalad prawnsalad commented Feb 11, 2019

Should having a description be a requirement? If I'm parsing a FAIL message on the client I should be always expect that the last argument is the freeform description otherwise I wouldn't know if it's an argument or description.

It may also be worth renaming the argument to avoid confusion. At first I thought they would be the arguments sent to the server after the command but that's not the case. As noticed in my previous paragraph in this comment, talking about the arguments is confusing when talking about an IRC message argument and an argument for this spec. Maybe something like.. contexts? There's probably something better.

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 11, 2019

Yes, description is a requirement. Servers could even send empty descriptions if they want to, but the final parameter is always a freeform text description. Same as 005 and its mandatory final param.

I could go with contexts, definitely.

@DanielOaks DanielOaks mentioned this pull request Feb 12, 2019
@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 14, 2019

So, it's pretty clear that FAIL is useful. Basically takes over the job of future ERR_ numerics, so that when I make a new command I can just use FAIL <cmd> to indicate that it didn't succeed.

There are two other sorts of cases that we may also want to cover: Informational messages (warnings while running a command / just general info), and a general success message (FAIL == command didn't complete successfully, == command did complete successfully).

I think the equivalent of a success message isn't as required, since a lot of the time the successful result of a command is some explicit successful reply batch or something of the like, but the informational messages + warnings sound be a good addition to this spec.


FAIL and NOTE, perhaps? Is there any word/message name that people can come up with which would work better than NOTE for general informational messages and warnings that should be displayed to users? Perhaps, FAIL, NOTE, SUCCESS?

@dgw
Copy link

@dgw dgw commented Feb 14, 2019

Probably worth having a WARN type, possibly instead of NOTE. To me, a "note" is easily ignored, but a "warn(ing)" needs to be handled/read.

@jesopo
Copy link

@jesopo jesopo commented Feb 14, 2019

Probably worth having a WARN type, possibly instead of NOTE. To me, a "note" is easily ignored, but a "warn(ing)" needs to be handled/read.

I think the point of a NOTE is to be ignored? I'd think maybe something like

NOTE * AUTH_SUCCESS :You are now logged in as jesopo

or maybe something like

:NickServ@services NOTE * AUTH_SUCCESS :You are now logged in as jesopo

@dgw
Copy link

@dgw dgw commented Feb 14, 2019

Both WARN and NOTE, then. One for actionable informational messages, the other for ignorables.

@jesopo
Copy link

@jesopo jesopo commented Feb 14, 2019

So...

  • FAIL for complete failure (e.g. SASL authentication failed)
  • WARN for a warning that needs attention/action (e.g. This nickname is registered. Please choose a different nickname)
  • NOTE for success or general information that can be ignored (e.g. You are now logged in as jesopo)

That sound right?

(sidenote - that WARN would help ircv3/ircv3-ideas#30)

@dgw
Copy link

@dgw dgw commented Feb 14, 2019

Sounds good to me.

I'm on the fence about using NOTE vs. the earlier suggestion of INFO (I think it was actually INFORM, pre-edit), but that's bikeshedding. Either one gets the same result.

@jesopo
Copy link

@jesopo jesopo commented Feb 14, 2019

It'd have to be INFORM rather than INFO as that already exists as a command

@dgw
Copy link

@dgw dgw commented Feb 14, 2019

Thought it might, but looking up specs is tricky enough when I'm not on mobile.

In that case, I'd lean toward NOTE because of its brevity (and the fact that it matches the lengths of FAIL & WARN).

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 14, 2019

Coolcool, I'd be good with FAIL, WARN, and NOTE. Will update the spec, if anyone else has input just chuck it here.

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 15, 2019

Initial round of updating to describe the FAIL, WARN, and NOTE messages. Will continue editing it a bit but if anyone spots issues, just add a comment!

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 15, 2019

Also a note: It's been suggested to use the same vendor-prefixing for codes that are not defined on the IRCv3 Reply Code Registry, similar to what we do for caps and tags.

If this happens, the addition of reply codes to the registry will be made very simple to encourage server implementers to push their things towards IRCv3. Rather than with tags and caps, where a full specification is required, the 'bar' will simply be submitting a note/issue/PR that the code is in use, the intended meaning of it (and how client software may wish to automagically respond to it), and a basic clarity check from us before agreeing to add it to the table.

Thoughts on this? Helps avoid fragmentation for clients, at the cost of requiring servers to register all their codes with us or vendor-prefix 'em.

@DanielOaks DanielOaks changed the title FAIL Message Standard Replies Extension Feb 15, 2019
@jesopo
Copy link

@jesopo jesopo commented Feb 15, 2019

if we're going to move some stuff from numerics/notices to FAIL/WARN/NOTE, do we need this to be a CAP?

@jwheare
Copy link
Member

@jwheare jwheare commented Feb 15, 2019

What’s moving?

@jesopo
Copy link

@jesopo jesopo commented Feb 15, 2019

that's an if but also e.g. ircv3/ircv3-ideas#30

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 15, 2019

We're not moving any existing numerics, so that won't be an issue. Notices... eh, I'm not too worried honestly.

@jwheare
Copy link
Member

@jwheare jwheare commented Feb 15, 2019

As I understand it, this stuff is more of a framework for other new specs to make use of, rather than a standalone spec.

But if there’s support for moving a lot of existing stuff over then it might be worth considering. A similar thing happened with message tags, but that also involved introducing quite a bit of new functionality. So I guess, wait and see what sort of proposals come up.

There’d be nothing to stop us introducing a cap in future as an extension to this framework.

@jesopo
Copy link

@jesopo jesopo commented Feb 15, 2019

Maybe my point should more be...

If we're looking at moving some NOTICEs to NOTEs, might it be worth having a CAP?

I can totally understand why we'd not need/want to move numerics but there's some useful information that's transmitted over NOTICE simply because that's all there was (e.g. think services, globals)

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 15, 2019

Definitely noted, for now my main focus is just on getting the feature itself fine, spec looking good, etc. An additional cap along those lines might be good to have, but something to be considered properly a bit further down the line.

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 21, 2019

Added the suggested OK message to round out the set. Now we have:

  • FAIL - Fatal processing error.
  • WARN - Non-fatal processing error.
  • NOTE - Informational note/response.
  • OK - Successful response.

Nice set of response messages to choose from, should let spec makers be able to use this for a decent proportion of their stuff.

@jwheare
Copy link
Member

@jwheare jwheare commented Feb 27, 2019

With OK are <code> and <description> still always required? I can imagine they could be superfluous there, as usually there's only one success state.

If they were optional for OK then we could probably use that instead of making a new verb like ACK for ircv3/ircv3-ideas#37

e.g.

S: PING xyz
C: @label=123 PONG xyz
S: @label=123 OK PONG

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 27, 2019

Yep, both are required for all these messages. One of my intentions for the suite of messages introduced by this specification is that they have exactly the same format to make parsing them very simple for clients, and so that servers have a consistent information-return interface.

Given the presence of the <context> parameters, making those two params optional isn't workable unless OK has a different format with no <context> parameters allowed, which can be done but I really don't want to.

@jwheare
Copy link
Member

@jwheare jwheare commented Feb 27, 2019

Right, following some discussion on IRC, I think we think OK is kind of an odd one out here. We already have a convention for successful command responses, the server uses the same verb, e.g. with NICK, etc. So I'm not sure there's a clear use case for it. The only case really was the ACK thing needed for labelled replies (ircv3/ircv3-ideas#37) but that can be specced separately I think.

(Hence the revert)

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Feb 27, 2019

After discussion on IRC, I agree wholeheartedly that the OK message isn't anywhere near as useful as I thought it could be. Totally not up for it being a part of this spec, as it almost always accompanies some additional information or other more particular return that doesn't need the code and description part of the format here. Given the considerations and questions that need to go into such a message, another spec is appropriate.

@prawnsalad
Copy link
Contributor

@prawnsalad prawnsalad commented Apr 22, 2019

While putting this in place for the BOUNCER proposal I've come across a use case whereby I need to send INFO for connect state changes (connected, disconnect, etc).

<command>: Indicates the user command which spawned this reply, or is * for messages initiated outside client commands (for example, an on-connect message).

With this language I shouldn't send NOTE BOUNCER ... since this may not be in response to a sent command. But it is in direction relation to the BOUNCER spec so perhaps relaxing the language a little to something like...

<command>: Indicates the user command which this reply is related to, or is * for messages initiated outside client commands (for example, an on-connect message).

I would imagine that NOTE ... lines would often not occur as a direct response to a sent command.

@prawnsalad
Copy link
Contributor

@prawnsalad prawnsalad commented Apr 22, 2019

Also, what was the reasoning for the OK response type to be removed? I'm hitting use cases where it would be useful at the moment, eg:

[c] BOUNCER delbuffer freenode #channel
[s] OK BOUNCER delbuffer freenode #channel :The buffer was deleted

@jwheare
Copy link
Member

@jwheare jwheare commented Apr 22, 2019

New wording sounds fine to me.

OK was dropped because convention is just to use the c2s command and an s2c verb. So the server could just echo the command back as a reply:

BOUNCER delbuffer freenode #channel :blah blah

@DanielOaks
Copy link
Member Author

@DanielOaks DanielOaks commented Apr 23, 2019

Sounds good and a really clean way to use this, new wording applied~

@DanielOaks DanielOaks mentioned this pull request Apr 29, 2019
13 tasks
@SadieCat
Copy link
Contributor

@SadieCat SadieCat commented Jul 25, 2019

There are several other specifications blocked by this one so if there's no more objections to this then can we finalise it?

@jwheare jwheare merged commit a881f7f into ircv3:master Aug 5, 2019
@slingamn slingamn mentioned this pull request Apr 26, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants