#3844 enhancement closed fixed (fixed)
Parse mIRC format codes
Reported by: | Jonathan Jacobs | Owned by: | Jonathan Jacobs |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | words | Keywords: | irc |
Cc: | Jean-Paul Calderone, jesstess, Thijs Triemstra | Branch: |
branches/parse-irc-formatting-3844-3
branch-diff, diff-cov, branch-cov, buildbot |
Author: | jonathanj |
Description
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.
Attachments (2)
Change History (69)
comment:1 Changed 13 years ago by
Keywords: | review removed |
---|
comment:2 Changed 13 years ago by
Cc: | Jean-Paul Calderone added |
---|---|
Owner: | changed from Jean-Paul Calderone to Jonathan Jacobs |
Changed 13 years ago by
Attachment: | parse-irc-formatting.diff added |
---|
comment:3 Changed 13 years ago by
Keywords: | review added |
---|
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
.
comment:4 Changed 13 years ago by
Owner: | Jonathan Jacobs deleted |
---|
comment:5 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jonathan Jacobs |
#3285 is out of review. Please re-submit this ticket when that one is accepted.
comment:6 Changed 12 years ago by
Cc: | jesstess added |
---|---|
Keywords: | review added |
Owner: | Jonathan Jacobs deleted |
comment:7 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jonathan Jacobs |
Can you put this in a branch and add @since markers for the new code in [source:trunk/twisted/words/protocols/irc.py]?
comment:8 Changed 12 years ago by
Author: | → jonathanj |
---|---|
Branch: | → branches/parse-irc-formatting-3844 |
(In [28041]) Branching to 'parse-irc-formatting-3844'
comment:9 Changed 12 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jonathan Jacobs to Thijs Triemstra |
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.
comment:10 follow-up: 12 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Thijs Triemstra to Jonathan Jacobs |
- What color standard does this parser implement? Add a reference to something to the docstring of something.
TextAttributesTest
is missing a docstring- Use
assertEquals
, notassertEqual
FormattedTextTests
is missing a docstringTextAttributes
should usetwisted.python.util.FancyEqMixin
- 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). - Several code paths through
TextAttributes.state_text
andTextAttributes.state_colorForeground
are untested. - 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.
comment:11 Changed 12 years ago by
comment:12 Changed 12 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jonathan Jacobs to Jean-Paul Calderone |
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.
comment:13 Changed 12 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
comment:14 Changed 12 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:15 follow-up: 19 Changed 12 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Jonathan Jacobs |
Status: | assigned → new |
- Regarding nesting
- 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: onlyassembleFormattedText
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). - 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 constructTextAttributes
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.
- I think you could support this while still keeping text separate from
- Line 3074, in
assembleFormattedText
,textAttrs = TextAttributes()
, is untested - What's with line 2947, in
state_text
,#self._attrs.toggle(formatName)
? - In
state_colorBackground
, the condition on line2995
is only half tested (the negative case isn't covered)
comment:17 Changed 11 years ago by
comment:18 Changed 11 years ago by
comment:19 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jonathan Jacobs to Jean-Paul Calderone |
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.
comment:20 follow-up: 24 Changed 11 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Jonathan Jacobs |
Thanks jonathanj.
- There are several things which aren't documented. Most especially:
twisted.words.protocols.irc.IRC_COLORS
,IRC_COLOR_NAMES
,CharacterAttributes
,attributes
, the properties ofCharacterAttribute
,toVT102
. Except, as you note, it's not actually VT102. So what is it? Why not have a different method?
- There's a minor merge conflict.
- Shouldn't
BlandCharacterAttribute
beDefaultCharacterAttribute
orNoCharacterAttributes
? 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.) - 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.
comment:21 Changed 11 years ago by
comment:22 Changed 11 years ago by
Branch: | branches/parse-irc-formatting-3844 → branches/parse-irc-formatting-3844-2 |
---|
(In [30730]) Branching to 'parse-irc-formatting-3844-2'
comment:23 Changed 11 years ago by
comment:24 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jonathan Jacobs to Glyph |
Replying to glyph:
- There are several things which aren't documented. Most especially:
twisted.words.protocols.irc.IRC_COLORS
,IRC_COLOR_NAMES
,CharacterAttributes
,attributes
, the properties ofCharacterAttribute
,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.
- There's a minor merge conflict.
- Shouldn't
BlandCharacterAttribute
beDefaultCharacterAttribute
orNoCharacterAttributes
? 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.
- 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.
comment:25 Changed 11 years ago by
Owner: | Glyph deleted |
---|
comment:26 Changed 11 years ago by
comment:27 Changed 11 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jonathan Jacobs |
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:
- 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. foldr
should be private (or, but probably not, it could be public intwisted.python.util
)- Do
OFF
,BOLD
, etc need to be public? Formatting should all go through theattributes
attribute, right? - 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.
- The states of
_FormattingState
should be upper case (coding standard). foreground
andbackground
are not documented in_FormattingState
's class docstringC{,}
should beC{","}
I guess, or just I{,} maybe? B{,}? It's not really code...- 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. :) CharacterAttribute
lost its == and != support (which I suppose means it was untested. still...)- The new uses of
super
are invalid :/ Please stick with explicitly named base class method invocation (use or disuse ofsuper
is effectively part of the public API, you can't switch compatibly) - in
text.py
, doesTEXT_COLORS
need to be public? - As far as the
toVT102
mess goes, the best solution that occurs to me is to make the method to invoke a parameter offlatten
. 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 mIRCifytwisted.words.protocols.irc
-formatted text right now, and you can only VT102ifytwisted.conch.text
-formatted text. Still, it avoids any backwards compatibility concerns, since the default can be to just keep callingtoVT102
. (Sad that we have this, a very similar version in Imaginary, and of course the two or three versions in Nevow.)
comment:28 Changed 11 years ago by
Plus one more, module docstring is not valid epytext, according to http://buildbot.twistedmatrix.com/builders/documentation/builds/952/steps/api-documentation/logs/stdio
comment:31 Changed 11 years ago by
comment:32 Changed 11 years ago by
comment:33 Changed 11 years ago by
comment:34 Changed 11 years ago by
comment:35 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jonathan Jacobs to Jean-Paul Calderone |
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:
- 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?
foldr
should be private (or, but probably not, it could be public intwisted.python.util
)- Do
OFF
,BOLD
, etc need to be public? Formatting should all go through theattributes
attribute, right?- in
text.py
, doesTEXT_COLORS
need to be public?
Changed these to private. Yes, attributes
is the only way formatting should take place.
- 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.
- The states of
_FormattingState
should be upper case (coding standard).foreground
andbackground
are not documented in_FormattingState
's class docstringC{,}
should beC{","}
I guess, or just I{,} maybe? B{,}? It's not really code...- The new uses of
super
are invalid :/ Please stick with explicitly named base class method invocation (use or disuse ofsuper
is effectively part of the public API, you can't switch compatibly)
Done.
- 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.
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.
- As far as the
toVT102
mess goes, the best solution that occurs to me is to make the method to invoke a parameter offlatten
. 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.
comment:36 Changed 11 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Jonathan Jacobs |
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.
comment:37 Changed 11 years ago by
Branch: | branches/parse-irc-formatting-3844-2 → branches/parse-irc-formatting-3844-3 |
---|
(In [33051]) Branching to 'parse-irc-formatting-3844-3'
comment:38 Changed 11 years ago by
comment:40 Changed 11 years ago by
comment:41 Changed 11 years ago by
Keywords: | irc review added |
---|---|
Owner: | Jonathan Jacobs deleted |
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.
comment:42 Changed 11 years ago by
Cc: | Thijs Triemstra added |
---|---|
Keywords: | review removed |
Owner: | set to Jonathan Jacobs |
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 welltwisted/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.
comment:43 Changed 11 years ago by
- "IRC client" in the first paragraph of the howto should link to the API docs.
comment:44 follow-up: 45 Changed 11 years ago by
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.
comment:45 follow-up: 52 Changed 11 years ago by
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.
comment:46 Changed 11 years ago by
Keywords: | review added |
---|---|
Owner: | Jonathan Jacobs deleted |
I made the documentation changes suggested and updated the @since
markers.
comment:47 Changed 10 years ago by
This is a review of the IRC part of the patch only.
- In
irc.py:3140
, wrapping theemit()
call inif self._buffer:
is unnecesary, as the whole ofemit()
is already wrapped in an identical call.
- 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.)
test_parseEmpty()
intest_irc.py
seems to be a duplicate oftest_parseEmptyString()
.
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.
- The
\x0f
formatting reset character doesn't clear colours.reset_formatting_tests.diff
contains a failing test for this.
- 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.)
Changed 10 years ago by
Attachment: | reset_formatting_tests.diff added |
---|
comment:48 Changed 10 years ago by
Keywords: | review removed |
---|
comment:49 Changed 10 years ago by
Owner: | set to Jonathan Jacobs |
---|
comment:50 Changed 10 years ago by
comment:51 Changed 10 years ago by
Summary: | Parse IRC format codes → Parse mIRC format codes |
---|
comment:52 Changed 10 years ago by
Replying to thijs:
Cool. One more thing, the introduced
@since: 11.0
should be updated for 12.
And now 12.1..
comment:53 Changed 10 years ago by
Thanks for the review.
Replying to jerith:
- In
irc.py:3140
, wrapping theemit()
call inif self._buffer:
is unnecesary, as the whole ofemit()
is already wrapped in an identical call.
Fixed.
- 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.
test_parseEmpty()
intest_irc.py
seems to be a duplicate oftest_parseEmptyString()
.
Indeed it is.
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
.
- 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.
- 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.
comment:55 follow-up: 56 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | Jonathan Jacobs deleted |
Looks like this should be back in review.
comment:56 follow-up: 57 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to 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>.
comment:57 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Jonathan Jacobs deleted |
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.
comment:58 follow-up: 59 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jonathan Jacobs |
- Update
@since
(again). - The docstring for the
attrs
argument of_textattributes._Attribute.serialize
refers to acharacter attribute instace
, but I think it may meanCharacterAttributes
?- In any case
- Are
copy
andwantOne
public? If not, at leastwantOne
could have a better name. In any case, the_textattributes
version can probably be private, with a public version in conch if necesary. _textattributes.flatten
says it returns vt102 compatible strings, but it seems it really depends on whatgetattr(attrs, 'attributeRenderer')
(?) returns!irc._FormattingState
:_buffer
and_attrs
could use some more description of how they are used.- It's not clear to me why
deepcopy
is being used. - How come you have tests for reduce?
assertEqual
is preferred toassertEquals
.- 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.
FormattedTextTests.test_assembleColor
doesn't test what it claims.- 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.
comment:59 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Jonathan Jacobs deleted |
Thank you for your thorough review, Tom!
Replying to tom.prince:
- The docstring for the
attrs
argument of_textattributes._Attribute.serialize
refers to acharacter attribute instace
, but I think it may meanCharacterAttributes
?
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".
- Are
copy
andwantOne
public? If not, at leastwantOne
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?
- 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.
- 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.
comment:60 Changed 9 years ago by
- 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.
- 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.
comment:62 follow-up: 64 Changed 9 years ago by
wantOne
only needs to exist in conch.- In
twisted/conch/insults/helper.py
:CharacterAttribute
probably wants to be deprecated. (seet.p.deprecate.deprecateModuleAttribute
).- I guess it is needed for
but if that is the only way to spell that, providing something like
flatten(<stuff>, CharacterAttribute())
assembleFormattedText
would seem appropriate.
- I guess it is needed for
- In 'twisted/conch/insults/text.py`: There are two paragraphs that begin "Non-color attributes ..."
- I hinted at this before, but I dont like the interface to
flatten
orserialize
(i.e. taking the name of an attribute to call).
comment:63 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jonathan Jacobs |
comment:64 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jonathan Jacobs to Tom Prince |
Replying to tom.prince:
- I hinted at this before, but I dont like the interface to
flatten
orserialize
(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.
comment:65 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Tom Prince to Jonathan Jacobs |
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.test_text
- 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)
test_flattenDeprecated
(see 1)- 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.
comment:66 follow-up: 67 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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.
comment:67 Changed 9 years ago by
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 :).
Do you have a reference for these codes?