Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

Add support for PROTOCTL command #15

Merged
merged 1 commit into from
Mar 29, 2015
Merged

Add support for PROTOCTL command #15

merged 1 commit into from
Mar 29, 2015

Conversation

Renegade334
Copy link
Contributor

No description provided.

@elazar
Copy link
Member

elazar commented Mar 28, 2015

Can you add a test case for this change?

@Renegade334
Copy link
Contributor Author

I considered this, but ended up ignoring it as:

  • PROTOCTL is an outgoing command only, so parser validation is really neither here nor there.
  • because of the non-standard syntax, the parser processes NAMESX as a "targets" parameter, which the test case would have to actively assert in order to pass. Asserting something that is not logically correct isn't very desirable.

@elazar
Copy link
Member

elazar commented Mar 29, 2015

PROTOCTL is outgoing from the client perspective, but not from the server perspective. I'm not discounting the possibility that a server implementation could be created using this parser.

While NAMESX may not be logically or, more specifically, semantically correct, the relevancy of the test is that the parser correctly parses the command and arguments; no more, no less.

All other commands are covered by test cases. I'm afraid I have to insist that this command not be an exception, non-standard or no.

+ define 005 RPL_ISUPPORT
@Renegade334
Copy link
Contributor Author

I've inserted a dummy empty capture into the regex to suppress the target parsing, which probably represents the best of both worlds. Rebased.

elazar added a commit that referenced this pull request Mar 29, 2015
Add support for PROTOCTL command
@elazar elazar merged commit d9e63ea into phergie:master Mar 29, 2015
@elazar
Copy link
Member

elazar commented Mar 29, 2015

That does indeed seem a reasonable compromise. Well done. Thank you very much for your contribution, cooperation, and persistence. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants