Protocol: Add undocumented message types

[taylorhansen]

Fix #8.

[taylorhansen]

Might still have to update Protocol.parse, but not sure if it's already taken care of in its code.

[scheibo]

Hey, thanks for taking this on! I don't think you need to change anything else (does subpkg test protocol pass? If so, I think you're good).

I think the main thing that jumps out at me is your handling of |noinit| and |hidelines|:

'|noinit|namerequired|': readonly ['noinit', 'namerequired', Message];
'|noinit|nonexistent|': readonly ['noinit', 'nonexistent', Message];
'|noinit|joinfailed|': readonly ['noinit', 'joinfailed', Message];
'|noinit|rename|': readonly ['noinit', 'rename', string, string];

Where I would have instead expected maybe:

'|noinit|': readonly ['noinit', 'namerequired' | 'nonexistent' | 'joinfailed', Message] 
	               | ['noinit', 'rename', string, string];

and similar for |hidelines|. However, I see you based things off of the odd |tournament| protocol command. |tournament| was actually only implemented in the weird compound way because of:

• how it is documented by PS (which sadly isn't something we can base things off of for these new commands as they are undocumented :/) and
• each of the tour subcommands tends to have vastly different arg types (|tournament| commands are basically all just different commands namespaced by |tournament|, as opposed to variations on a single command).

The way I see |noinit| and |hidelines| here is different - theyre still doing one thing (not initializing a room or hiding lines), they just have that action further broken down to provide more specific error messages/behavior, whereas the |tournament| commands are each doing completely different things. Does this distinction make sense? WDYT?

[scheibo]

Note: as mentioned, the entire test suite is failing, but at least it still shows the protocol package you've changed is OK :). So long as that package's tests are green this PR shouldn't worry about the rest of the suite (really I should try to find a way to have two builds - one which links and one which doesn't, and only block PRs on the one which links, but maybe I'll look into that later).

[taylorhansen]

Thanks for the review! I should be able to finish everything later today if that doesn’t interfere with the release schedule.

[scheibo]

No worries, I will wait for this to land before releasing. It doesn't look like there were any tier shifts this month anyway :)

[scheibo]

Super close! Thanks for making these changes.

[scheibo]

What's going on with this type? The ...[] is kind of confusing to me, as it the array nesting. Can this be simplified?

[taylorhansen]

It's parsed as ...([] | [Message] | [Message, string]) due to operator precedence and tuple spreading, though some style guides enforce the parentheses anyway for clarity. Could do that or manually split it into a 3-array union, your call.

[scheibo]

I think the parens will help at the bare minimum.

['tempnotify', TempNotifyName, Message] | ['tempnotify', TempNotifyName, Message, Message] | ['tempnotify', TempNotifyName, Message, Message, string] seems the most clear (unless I'm mistaking what this type actually is?). ['tempnotify', TempNotifyName, Message, Message?, string?] seems less precise but possibly how I might type this if I were doing it, as I'm not positive the extra precision really matters. Up to you which route you want to go, but please add parens if you want to keep the current approach :)

[taylorhansen]

The ? suffix allows for undefined and/or skipping the tuple element, so that might not be ideal. Probably best to expand it into the type union then.

Will do this for |expire| as well.

[scheibo]

Yeah, it's less accurate, but we're also trying to put super rigorous types on top of something which is basically just raw strings on the PS side, so I'm usually not to bothered about dropping an ? for convenience because PS is more likely to break things in the protocol than the undefined is going to cause problems for people :).

Technically its also the case that for like ['expire'] you can still index at 1 and get undefined, so really the main difference with ? is that you're basically implying the array should be length 2 with the second element occasionally undefined, though if I really wanted to be explicit about that I'd probably go Message | undefined (its also the case that nothing in the protocol can be undefined because we can't translate undefined over the wire).

tl;dr pedantically yes, the ? implies some things different than the union, but in practice I'm not too hot and bothered by it in the context of the PS protocol :)

[scheibo]

Thanks! I'll import the latest PS changes and cut a new release.