Parse mIRC format codes

[Jonathan Jacobs]

IRC text can contain formatting codes, used as a basic kind of markup. It would be useful to be able to correctly parse formatted text, either to strip the formatting codes or to interpret and use them in an IRC client implementation.

[Jean-Paul Calderone]

Do you have a reference for these codes?

[Jonathan Jacobs]

Attached a patch that introduces a parser (that turns formatted messages into structured information) and an assembler (that turns structured information into formatted messages.) This patch depends on #3285 for CommandDispatcherMixin.

[Jean-Paul Calderone]

#3285 is out of review. Please re-submit this ticket when that one is accepted.

[Thijs Triemstra]

Can you put this in a branch and add @since markers for the new code in [source:trunk/twisted/words/protocols/irc.py]?

[Jonathan Jacobs]

(In [28041]) Branching to 'parse-irc-formatting-3844'

[Jonathan Jacobs]

Glyph once suggested using twisted.conch.insults.text instead of reinventing text attribute stuff, which seems like a noble suggestion but I'm not sure whether it's a good one.

[Jean-Paul Calderone]

1. What color standard does this parser implement? Add a reference to something to the docstring of something.
2. TextAttributesTest is missing a docstring
3. Use assertEquals, not assertEqual
4. FormattedTextTests is missing a docstring
5. TextAttributes should use twisted.python.util.FancyEqMixin
6. Having TextAttributes.color as a list seems a bit odd to me. A tuple would make more sense, or an object with two attributes (foreground and background).
7. Several code paths through TextAttributes.state_text and TextAttributes.state_colorForeground are untested.
8. Some further documentation about how to use TextAttributes to generate a colorful string would probably be useful.

I agree that being able to share code between words and conch would be neat. It's not immediately obvious how one might go about that, though. Someone can worry about it later.

Another thing that strikes me is that parseFormattedText must have abysmal performance. 3+ method calls per byte is crazy. Not that I really care so much about the performance. However, I expect that someone is going to complain at some point. We may as well make sure the test coverage is 100% now, so that things don't get broken by a later optimization. Line coverage is pretty good now, but there are no tests for, eg, reverse video support. It'd be nice to cover everything like that now.

[Jonathan Jacobs]

(In [28072]) Use FancyEqMixin for TextAttributes, add missing docstrings and fix coding style infractions. refs #3844

[Jonathan Jacobs]

I've addressed all of your review points.

One thing I noticed while writing examples for creating formatted text is that it is not currently possible to nest formatting. A solution for this is moving the text part into TextAttributes and allowing it to be str or a list of TextAttributes. The reason I suggest this is that implementing nesting later while maintaining backward compatibility is going to be awkward.

[Jean-Paul Calderone]

1. Regarding nesting
   1. I think you could support this while still keeping text separate from TextAttributes instances. Consider a structure like [(attr, [(attr, text), (attr, text])]. This can be introduced in a compatible way: only assembleFormattedText needs to change. As I understand things, parseFormattedText would never need to return a structure with such nesting, since any such nesting can also be represented as a flat structure (and enhancing the parser to be able to recognize nesting would be challenging anyway).
   2. However, once we start thinking about supporting nesting, then I also begin to think about how to share code with twisted.conch.insults.text... Even if we don't do this now, I wonder if there's a way we can leave the door open for it in the future. The conch code makes almost all of the representation objects private and presents an alternate API for constructing them. This probably doesn't map well to the mirc color codes, though, since in conch the colors are named and limited to the 8 historically supported on terms. Oh, dang, I just read the mirc page about what the rest of the values in that range mean. sigh. Well great, that maps exactly, then. So what's my point? Um, how would you feel about just making the canonical way to construct TextAttributes API compatible with the way it's done in Conch (but if you don't want to bother sharing the implementation or representation, that's fine). Alternatively, continue to ignore this and we can introduce the compatible API later.
2. Line 3074, in assembleFormattedText, textAttrs = TextAttributes(), is untested
3. What's with line 2947, in state_text, #self._attrs.toggle(formatName)?
4. In state_colorBackground, the condition on line 2995 is only half tested (the negative case isn't covered)

[Jonathan Jacobs]

(In [30600]) Support nested formatting, refs #3844.

[Jonathan Jacobs]

(In [30629]) Make twisted.conch.insults.text and twisted.conch.insults.helper friendlier to use outside of conch, refs #3844.

[Jonathan Jacobs]

(In [30630]) Use an API compatible with Conch for IRC formatting, refs #3844.

[Jonathan Jacobs]

Replying to exarkun:

Um, how would you feel about just making the canonical way to construct TextAttributes API compatible with the way it's done in Conch (but if you don't want to bother sharing the implementation or representation, that's fine). Alternatively, continue to ignore this and we can introduce the compatible API later.

Okay, so I've switched this to an API that is compatible with the Conch API. I think it is better to do this now than to try and introduce compatibility later.

[Glyph]

Thanks jonathanj.

1. There are several things which aren't documented. Most especially:
   1. twisted.words.protocols.irc.IRC_COLORS, IRC_COLOR_NAMES, CharacterAttributes, attributes, the properties of CharacterAttribute, toVT102. Except, as you note, it's not actually VT102. So what is it? Why not have a different method?
2. There's a minor merge conflict.
3. Shouldn't BlandCharacterAttribute be DefaultCharacterAttribute or NoCharacterAttributes? Blandness is a matter of taste; my green-on-black terminals aren't bland, but they are default :). (Unless some standard uses the term 'bland' for this sort of thing, in which case just add a reference to that and ignore me.)
4. I'm not entirely clear on how this functionality would be used by an application. Can you add a brief example or some documentation?

I'll try to re-review quickly when you've addressed this stuff, feel free to ping me.

[Jonathan Jacobs]

(In [30726]) Rename BlandCharacterAttribute to DefaultCharacterAttribute, refs #3844.

[Jonathan Jacobs]

(In [30730]) Branching to 'parse-irc-formatting-3844-2'

[Jonathan Jacobs]

(In [30736]) Document CharacterAttributes, attributes and add some module-level docstrings explaining how to create and parse formatted text, refs #3844.

[Jonathan Jacobs]

Replying to glyph:

1. There are several things which aren't documented. Most especially:
   1. twisted.words.protocols.irc.IRC_COLORS, IRC_COLOR_NAMES, CharacterAttributes, attributes, the properties of CharacterAttribute, toVT102. Except, as you note, it's not actually VT102. So what is it? Why not have a different method?

I've prefixed IRC_COLORS and IRC_COLOR_NAMES with an _, and added a comment to each. These are strictly implementation details. I could add a docstring but it's not going to say more than the comment.

toVT102 comes from the conch infrastructure being reused. Almost all of this infrastructure is private although I'm a little weary of changing it without at least preserving backwards compatibility. I was unable to come up with a reasonable name that suited both conch and IRC. Perhaps serialize and have the conch implementation call toVT102, does this seem reasonable?

I've documented the other things you mentioned.

1. There's a minor merge conflict.
2. Shouldn't BlandCharacterAttribute be DefaultCharacterAttribute or NoCharacterAttributes? Blandness is a matter of taste; my green-on-black terminals aren't bland, but they are default :). (Unless some standard uses the term 'bland' for this sort of thing, in which case just add a reference to that and ignore me.)

Fixed.

1. I'm not entirely clear on how this functionality would be used by an application. Can you add a brief example or some documentation?

The docstring of assembleFormattedText contains quite a lot of detail, although I suppose it is not directly visible. I've added some prose to the module-level docstring (that also links to the useful functions) outlining what the process is.

[Jean-Paul Calderone]

I don't like the new Twisted Words dependency on (private parts of )Twisted Conch. I think the way we should be sharing code across subprojects is to put it into core and then use it from Conch and Words. It can still be private in core, with comments/docstrings indicating it is for internal Twisted use only (whereas generally I would say that "normal" private APIs should rarely be used from different modules and basically never from different projects).

Aside from that:

1. The color stuff in the irc module docstring is somewhat jarring. Can we move it behind an introduction of some sort? It is an IRC module after all, not a text formatting module.
2. foldr should be private (or, but probably not, it could be public in twisted.python.util)
3. Do OFF, BOLD, etc need to be public? Formatting should all go through the attributes attribute, right?
4. I think in some places in docstrings where you have :: you just want :. :: is for preformatted text. So, for example, lists don't render as lists if they follow a ::. Maybe the right rule to use is that :: should only introduce code snippets.
5. The states of _FormattingState should be upper case (coding standard).
6. foreground and background are not documented in _FormattingState's class docstring
7. C{,} should be C{","} I guess, or just I{,} maybe? B{,}? It's not really code...
8. I think test_assemble is more like 5 or 6 test methods squashed together. That you were tempted to write "the correct control codes" in the docstring is a hint. :)
9. CharacterAttribute lost its == and != support (which I suppose means it was untested. still...)
10. The new uses of super are invalid :/ Please stick with explicitly named base class method invocation (use or disuse of super is effectively part of the public API, you can't switch compatibly)
11. in text.py, does TEXT_COLORS need to be public?
12. As far as the toVT102 mess goes, the best solution that occurs to me is to make the method to invoke a parameter of flatten. It's the caller who knows what kind of output they want. But that's probably of no practical value now, since you can only mIRCify twisted.words.protocols.irc-formatted text right now, and you can only VT102ify twisted.conch.text-formatted text. Still, it avoids any backwards compatibility concerns, since the default can be to just keep calling toVT102. (Sad that we have this, a very similar version in Imaginary, and of course the two or three versions in Nevow.)

[Jean-Paul Calderone]

Plus one more, module docstring is not valid epytext, according to ​http://buildbot.twistedmatrix.com/builders/documentation/builds/952/steps/api-documentation/logs/stdio

[Jonathan Jacobs]

(In [30812]) Make private things private, refs #3844.

[Jonathan Jacobs]

(In [30814]) Make _FormattingState states uppercase, refs #3844.

[Jonathan Jacobs]

(In [30816]) Allow callers of insults.text.flatten to specify which method to call when rendering character attributes, refs #3844.

[Jonathan Jacobs]

(In [30817]) Make twisted.words.protocol.irc's docstring valid epytext, refs #3844.

[Jonathan Jacobs]

(In [30818]) Make CharacterAttribute comparable again, refs #3844.

[Jonathan Jacobs]

(In [30820]) Remove uses of super() and use explicit base class method calls, refs #3844.

[Jonathan Jacobs]

Replying to exarkun:

I don't like the new Twisted Words dependency on (private parts of )Twisted Conch. I think the way we should be sharing code across subprojects is to put it into core and then use it from Conch and Words. It can still be private in core, with comments/docstrings indicating it is for internal Twisted use only (whereas generally I would say that "normal" private APIs should rarely be used from different modules and basically never from different projects).

That sounds fine to me, where should I put this code then and how much code should I move to core?

Aside from that:

1. The color stuff in the irc module docstring is somewhat jarring. Can we move it behind an introduction of some sort? It is an IRC module after all, not a text formatting module.

I would agree that it is jarring, having stuff like "Test coverage needs to be better" is also jarring. What did you have in mind when you said "introduction"? Some more prose in the docstring? What kinds of things should the introduction contain?

1. foldr should be private (or, but probably not, it could be public in twisted.python.util)
2. Do OFF, BOLD, etc need to be public? Formatting should all go through the attributes attribute, right?
3. in text.py, does TEXT_COLORS need to be public?

Changed these to private. Yes, attributes is the only way formatting should take place.

1. I think in some places in docstrings where you have :: you just want :. :: is for preformatted text. So, for example, lists don't render as lists if they follow a ::. Maybe the right rule to use is that :: should only introduce code snippets.
2. The states of _FormattingState should be upper case (coding standard).
3. foreground and background are not documented in _FormattingState's class docstring
4. C{,} should be C{","} I guess, or just I{,} maybe? B{,}? It's not really code...
5. The new uses of super are invalid :/ Please stick with explicitly named base class method invocation (use or disuse of super is effectively part of the public API, you can't switch compatibly)

Done.

1. I think test_assemble is more like 5 or 6 test methods squashed together. That you were tempted to write "the correct control codes" in the docstring is a hint. :)

I broke this up into several smaller tests.

1. CharacterAttribute lost its == and != support (which I suppose means it was untested. still...)

I've added a compareAttributes attribute to all the CharacterAttribute types and tests for testing for equality.

1. As far as the toVT102 mess goes, the best solution that occurs to me is to make the method to invoke a parameter of flatten. It's the caller who knows what kind of output they want.

Good idea. Done.

Replying to exarkun:

Plus one more, module docstring is not valid epytext, according to ​http://buildbot.twistedmatrix.com/builders/documentation/builds/952/steps/api-documentation/logs/stdio

Fixed.

[Jean-Paul Calderone]

That sounds fine to me, where should I put this code then and how much code should I move to core?

Move as much as is shared by words and conch now. As for where to move it, I dunno, something private in twisted.python I suppose.

[Jonathan Jacobs]

(In [33051]) Branching to 'parse-irc-formatting-3844-3'

[Jonathan Jacobs]

(In [33064]) Move code shared by insults and the IRC text formatting to twisted.python._textattributes, refs #3844.

[Jonathan Jacobs]

(In [33072]) Create a howto for IRC text formatting, refs #3844.

[Jonathan Jacobs]

(In [33073]) Remove documentation in the source code that is now in the howto, refs #3844.

[Jonathan Jacobs]

Okay, so I've moved the code shared by insults and IRC to twisted.python._textattributes, I've left most of the tests in Conch (since all of Conch's tests indirectly test all the private stuff and untangling all those tests seems kind of hard. Maybe there should be some more tests?)

I've also added a howto for the IRC client and written a section about text formatting. I filed bug #5363 for expanding the client howto.

[Thijs Triemstra]

• twisted/python/_textattributes.py is missing a copyright header and module docstring. Even though it's private a module-level @since: 12.0 would be nice as well
• twisted/python/test/test_textattributes.py is missing a copyright header and module docstring
• in doc/words/howto/index.xhtml:
  • the "Twisted" in "Using the Twisted IRC client" can go or should be "Words" or "Twisted Words".
• in doc/words/howto/ircclient.xhtml:
  • mIRC could have a link to ​http://en.wikipedia.org/wiki/MIRC or some other (permanent) site with a good mIRC description.
  • In "used by Twisted Insults.", the Insults could be linked and/or replaced with a API reference to the relevant insults module/class.
  • Change "Since other IRC clients can, and do, format text" into "Most IRC clients format text" etc
  • add a second sentence for "This can be accomplished with" etc, instead of using ", this can be accomplished with" etc.

[Thijs Triemstra]

• "IRC client" in the first paragraph of the howto should link to the API docs.

[Jean-Paul Calderone]

Even though it's private a module-level @since: 12.0 would be nice as well

Please no. These markers are for public APIs. They do not make sense for private APIs, which have no compatibility requirements.

[Thijs Triemstra]

Replying to exarkun:

Even though it's private a module-level @since: 12.0 would be nice as well

Please no. These markers are for public APIs. They do not make sense for private APIs, which have no compatibility requirements.

Cool. One more thing, the introduced @since: 11.0 should be updated for 12.

[Jonathan Jacobs]

I made the documentation changes suggested and updated the @since markers.

[Jeremy Thurgood]

This is a review of the IRC part of the patch only.

1. In irc.py:3140, wrapping the emit() call in if self._buffer: is unnecesary, as the whole of emit() is already wrapped in an identical call.

2. Docs for the handling of colour indexes > 15 would be a good idea. (The "spec" mentions two different ways to handle this, and it would be good to know which was chosen and why.)

3. test_parseEmpty() in test_irc.py seems to be a duplicate of test_parseEmptyString().

4. assertParsesTo() asserts that a parse/assemble round trip is the same as assembly of the expected parameter. This means that a bug in the assembler can mask a bug in the parser for tests that use this. I can't think of a good way to avoid this without making the tests fragile to semantically-equivalent changes in the parser output, but I think the name of the method is currently a little deceptive.

5. The \x0f formatting reset character doesn't clear colours. reset_formatting_tests.diff contains a failing test for this.

6. I don't think we want to start all formatted strings with \x0f. The overwhelmingly common case is that there is no formatting, and clients that don't understand formatting will struggle to handle these. (This means you'd need special handling to join potentially formatted-and-flattened strings without leaking formatting, but I think documenting that and possibly providing a helper function is enough for that.)

[Jonathan Jacobs]

(In [33591]) Tests and fixes for bug reported by jerith, refs #3844.

[Thijs Triemstra]

Replying to thijs:

Cool. One more thing, the introduced @since: 11.0 should be updated for 12.

And now 12.1..

[Jonathan Jacobs]

Thanks for the review.

Replying to jerith:

1. In irc.py:3140, wrapping the emit() call in if self._buffer: is unnecesary, as the whole of emit() is already wrapped in an identical call.

Fixed.

2. Docs for the handling of colour indexes > 15 would be a good idea. (The "spec" mentions two different ways to handle this, and it would be good to know which was chosen and why.)

I added some comments around this code, some API docs to parseFormattedText and a sentence to the howto.

3. test_parseEmpty() in test_irc.py seems to be a duplicate of test_parseEmptyString().

Indeed it is.

4. assertParsesTo() asserts that a parse/assemble round trip is the same as assembly of the expected parameter. This means that a bug in the assembler can mask a bug in the parser for tests that use this. I can't think of a good way to avoid this without making the tests fragile to semantically-equivalent changes in the parser output, but I think the name of the method is currently a little deceptive.

Renamed this to assertAssembledEqually.

5. The \x0f formatting reset character doesn't clear colours. reset_formatting_tests.diff contains a failing test for this.

Thanks for the test cases, I've made these pass.

6. I don't think we want to start all formatted strings with \x0f. The overwhelmingly common case is that there is no formatting, and clients that don't understand formatting will struggle to handle these. (This means you'd need special handling to join potentially formatted-and-flattened strings without leaking formatting, but I think documenting that and possibly providing a helper function is enough for that.)

I think in the interests of correctness \x0f should be emitted at the beginning of assembled text always. In terms of the implementation, it becomes difficult to optimize away the control codes with the current text attribute implementation. As far as your concerns go: If you're worried about clients that don't support formatted text then you should probably never use formatted text (or always strip formatting), if you're not worried about them then there is no problem. Since there is no solid use case at the moment, I think it is best to leave this behavior until someone reports bug or provides a helpful suggestion. I've added a sentence to the howto and API docs noting this behavior.

[Jonathan Jacobs]

(In [33598]) Update @since to 12.1, refs #3844.

[Thijs Triemstra]

Looks like this should be back in review.

[Jonathan Jacobs]

Replying to thijs:

Looks like this should be back in review.

Not quite yet, there is still the matter of fixing the bug I wrote to the list about <​http://twistedmatrix.com/pipermail/twisted-python/2012-February/025270.html>.

[Jonathan Jacobs]

Replying to jonathanj:

Replying to thijs:

Looks like this should be back in review.

Not quite yet, there is still the matter of fixing the bug I wrote to the list about <​http://twistedmatrix.com/pipermail/twisted-python/2012-February/025270.html>.

After some thinking I've concluded that fixing the conch issue is probably outside the scope of this ticket. For now the IRC formatting can produce inefficient, but correct, output that can be improved in a later ticket.

Sorry about holding this ticket up for so long.

[Tom Prince]

1. Update @since (again).
2. The docstring for the attrs argument of _textattributes._Attribute.serialize refers to a character attribute instace, but I think it may mean CharacterAttributes?
   • In any case
3. Are copy and wantOne public? If not, at least wantOne could have a better name. In any case, the _textattributes version can probably be private, with a public version in conch if necesary.
4. _textattributes.flatten says it returns vt102 compatible strings, but it seems it really depends on what getattr(attrs, 'attributeRenderer')(?) returns!
5. irc._FormattingState: _buffer and _attrs could use some more description of how they are used.
6. It's not clear to me why deepcopy is being used.
7. How come you have tests for reduce?
8. assertEqual is preferred to assertEquals.
9. I wonder if it would make sense to enhance the equality to look through NormalAttr? That seems like it would mean that things that get assembled the same compare equal.
10. FormattedTextTests.test_assembleColor doesn't test what it claims.
11. The howto should at least have a plcaeholder, indicating that the text-formating bit is just the only bit that has been written, rather than the impression that irc is about text formating.

[Jonathan Jacobs]

Thank you for your thorough review, Tom!

Replying to tom.prince:

2. The docstring for the attrs argument of _textattributes._Attribute.serialize refers to a character attribute instace, but I think it may mean CharacterAttributes?

No, I'm pretty sure this is supposed to be an instance of CharacterAttribute, that's what the example in the documentation says and the code expects to be able to call wantOne on it. However, after our conversation on IRC I took exarkun's advice and renamed CharacterAttribute to FormattingState and updated the docstrings to talk about "formatting states" instead of the more confusing "character attributes".

3. Are copy and wantOne public? If not, at least wantOne could have a better name. In any case, the _textattributes version can probably be private, with a public version in conch if necesary.

As far as I can tell they probably count as public. I've renamed wantOne to _withAttribute but left a wantOne stub to emulate the old behaviour. Should I emit a deprecation warning?

6. It's not clear to me why deepcopy is being used.

Whoops, I guess this was from the old code before I adopted the conch-style text formatting structure.

9. I wonder if it would make sense to enhance the equality to look through NormalAttr? That seems like it would mean that things that get assembled the same compare equal.

I didn't follow this point, would you mind explaining in more detail what you're thinking?

I believe I've addressed all of your other points.

​build results.

[Tom Prince]

3. Are copy and wantOne public? If not, at least wantOne could have a better name. In any case, the _textattributes version can probably be private, with a public version in conch if necesary.

As far as I can tell they probably count as public. I've renamed wantOne to _withAttribute but left a wantOne stub to emulate the old behaviour. Should I emit a deprecation warning?

I'd suggest yes.

9. I wonder if it would make sense to enhance the equality to look through NormalAttr? That seems like it would mean that things that get assembled the same compare equal.

I didn't follow this point, would you mind explaining in more detail what you're thinking?

As far as I can tell, the main reason for assertAssembledEqually, is that there can be meaningless differences in how attribute trees are constructed, and you want to ignore them. I was wondering if it would make sense to make trees that are different only in meaningless ways compare equal, rather than depending on assembly to hide those differences.

[Tom Prince]

(In [38051]) Remove some trailing whitespace.

Refs: #3844

[Tom Prince]

1. wantOne only needs to exist in conch.
2. In twisted/conch/insults/helper.py: CharacterAttribute probably wants to be deprecated. (see t.p.deprecate.deprecateModuleAttribute).
   • I guess it is needed for

     flatten(<stuff>, CharacterAttribute())
     

     but if that is the only way to spell that, providing something like assembleFormattedText would seem appropriate.
3. In 'twisted/conch/insults/text.py`: There are two paragraphs that begin "Non-color attributes ..."
4. I hinted at this before, but I dont like the interface to flatten or serialize (i.e. taking the name of an attribute to call).

[Jonathan Jacobs]

Replying to tom.prince:

4. I hinted at this before, but I dont like the interface to flatten or serialize (i.e. taking the name of an attribute to call).

I decided to introduce a assembledFormattedText function for Conch too, this avoids users having to know about renderer names and character attribute instances. I deprecated twisted.conch.insults.text.flatten too, but left the API for _textattributes.flatten as is.

I believe I have addressed all of your other commentary too.

[Tom Prince]

1. test_wantOneDeprecated: You don't need to check that there are no warnings to begin with. trial keep track of warnings on a per-test basis.
2. test_text
   1. Thanks for cleaning this up. Since your here adding docstring anyway, could make them describe what is being tested (see #6301) rather than just being a noun string. (Sorry)
   2. test_flattenDeprecated (see 1)
   3. It occurs to me that deprecating a function using deprecatedModuleAttribute is odd, but it does save having a forwarding function.

Other than that, this looks good. Please merge after addressing 1+2.1+2.2 and a buildbot run.

[Jonathan Jacobs]

(In [38110]) Merge parse-irc-formatting-3844-3

Author: jonathanj Reviewer: exarkun, glyph, jerith, tom.prince Fixes: #3844

Introduce a function, twisted.words.protocols.irc.assembleFormattedText, for creating mIRC-formatted text from a formatting structure; and a function, twisted.words.protocols.irc.stripFormatting, for removing mIRC-formatting from text.

[Glyph]

Replying to jonathanj:

Introduce a function, twisted.words.protocols.irc.assembleFormattedText, for creating mIRC-formatted text from a formatting structure; and a function, twisted.words.protocols.irc.stripFormatting, for removing mIRC-formatting from text.

Sorry; just going off this comment, not the diff, but it seems like a piece is missing here; in order to present mIRC-formatted text, don't we also need a thing that can convert it to some data structure which the client can inspect? Do we need a separate ticket for that or did you just forget to mention it?

Hypothetically I'd love to have a Twisted server that could accurately bridge formatting between mIRC colors on IRC and HTML fragments in XMPP :).