You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is a bit of a weird one, because it's fundamentally invalid and I'm not sure how you would handle that in the YAML -- empty verb key perhaps, and let it fail somewhere else in the stack?
Example (broken) input: :foo.
This raises a (cryptic) exception in goshuirc 0.4.0's RFC1459Message, so I think it may be important to test for:
goshuirc-irc/girc/ircreactor/envelope.py in from_message(cls, message)
104 s = s[1:]
105
--> 106 verb = s[0].upper()
107 original_params = s[1:]
108 params = []
IndexError: list index out of range
Similar in nature to #12, which is for blank tag/source.
Yeah it definitely makes sense to have a test in here, if nothing else for safety so that server-side parsers also don't throw unexpected exceptions when getting these messages. We could either add a valid key to all the tests, matching how the hostname tests are laid out, or we could just create a new root element so there's tests and failing-tests. Not sure which one's cleaner but definitely makes sense to add this, thanks for the yell mate.
Going one step further, it could potentially make sense to also test for all kinds of other exceptional conditions of that sort, e.g. verbs that contain symbols/numbers/non-ASCII characters. Basically anything outside 2812's BNF definition of \w+|\d{3}. Perhaps that's a bit too much of a scope creep for these tests, however?
No worries! Yeah I'd see that as alright, as an FYI I tend to prefer matching real-world behaviour rather than the RFCs, so I'd wanna confirm there's no commands out there right now that include numbers/_. Just to be safe, since those seem like the kind of limits that servers may have ignored over the years~
Activity
DanielOaks commentedon Apr 16, 2020
Yeah it definitely makes sense to have a test in here, if nothing else for safety so that server-side parsers also don't throw unexpected exceptions when getting these messages. We could either add a
validkey to all the tests, matching how the hostname tests are laid out, or we could just create a new root element so there'stestsandfailing-tests. Not sure which one's cleaner but definitely makes sense to add this, thanks for the yell mate.paravoid commentedon Apr 16, 2020
Thanks for the quick response!
Going one step further, it could potentially make sense to also test for all kinds of other exceptional conditions of that sort, e.g. verbs that contain symbols/numbers/non-ASCII characters. Basically anything outside 2812's BNF definition of
\w+|\d{3}. Perhaps that's a bit too much of a scope creep for these tests, however?DanielOaks commentedon Apr 16, 2020
No worries! Yeah I'd see that as alright, as an FYI I tend to prefer matching real-world behaviour rather than the RFCs, so I'd wanna confirm there's no commands out there right now that include numbers/
_. Just to be safe, since those seem like the kind of limits that servers may have ignored over the years~Handle blank commands in IRCMessages
Handle blank commands in IRCMessages