I just glanced at https://github.com/twisted/twisted/pull/1417 <https://github.com/twisted/twisted/pull/1417> after it was merged and noticed a few problems:
- It exposes a new top-level name rather than setting the protocol version as a parameter - The NEWS files are malformed which is going to lead to some confusing duplication in the changelog - AMPv2 itself does not appear to be "standardized" - the page at https://amp-protocol.net/conversations_v2.html <https://amp-protocol.net/conversations_v2.html> appears to be a preliminary suggestion for how long values might be handled rather than something broadly implemented; for one thing, I'd never heard of it before and for another the site itself doesn't link the document. This would be fine as a first draft, but I think it needs to be given some more revisions, given that (for example) there are a number of backwards-compatible ways to do this. - The layering is wrong because it puts the protocol-parsing into a leaf class in the hierarchy, when the parsing logic was deliberately isolated to a lower level to facilitate different framing mechanisms. - As JP pointed out, the tests have a potential bug where they can leak errors between cases. I don't normally like to revert folks' work once it's been reviewed and accepted, particularly when there's no process violation (broken CI, lack of code coverage etc), but I'm particularly concerned about lending Twisted's imprimatur to this protocol extension as a whole new version without much more context on who is implementing it and what other options were considered, particularly when I personally (nominally the inventor of AMP) don't like the direction it has taken :). What do other folks think? Anyone else have more of a finger on the pulse of where the "v2" conversation has been happening? -glyph
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
