event: Explicitly ignore each remaining event type; check for exhaustiveness #5310
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Awkwardly this actually requires the cases to use string literals directly, rather than the named constants. I prefer named constants for the sake of greppability... but when required to choose, checking for errors is more important. The fact that Flow passes after this change means that the `switch` is covering all the event types our API code knows about. There are a number of others the server can send, though. Next we'll add all of those to the list, and explicitly call out the ones we're ignoring.
gnprice
added
the
a-data-sync
42 tasks
With comments about in what circumstances we'll need to start
handling them.
Generated the list automatically from the API metadata, like so:
$ python -c 'if 1:
import yaml, json, sys
z = yaml.safe_load(open("../zulip/zerver/openapi/zulip.yaml"))
json.dump(z, sys.stdout)
' | jq '
.paths["/events"].get.responses["200"].content["application/json"]
.schema.allOf[2].properties.events.items.oneOf
[] | ((.properties // .allOf[1].properties).type.allOf[1].enum[0] // .)
' -r \
| sort -u \
| perl -lne 'print " $_: null,"'
(That `jq` formula is a bit brittle to the details of how our
openapi/zulip.yaml is written. If you're trying it in the future and
that has changed in a way that breaks this, the `// .` should cause
the whole objects affected to get spit out. In that case, delete the
`| sort -u` to see more clearly for debugging.)
These were never documented, were never needed for mobile, and are now obsolete. But for the moment we still support servers that send them, so we want to explicitly ignore them.
chrisbobbe
approved these changes
Mar 25, 2022
|
|
||
| case EventTypes.restart: | ||
| case EventTypes.stream: | ||
| case 'restart': |
There was a problem hiding this comment.
Awkwardly this actually requires the cases to use string literals
directly, rather than the named constants. I prefer named constants
for the sake of greppability... but when required to choose, checking
for errors is more important.
Huh, yeah. Even this doesn't seem to work:
case (EventTypes.restart: 'restart'):Shrug, ah, well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should completely eliminate errors of the form "Unhandled Zulip API event type", so that when they show up again it's because of some new event type added on the server.
It also gives us a bit more of an inventory of what's needed for #3408. (Though really the main part of that task is events that we already know exist and just don't completely handle, and this doesn't affect those.)