edgewall / genshi

Python toolkit for generation of output for the web
http://genshi.edgewall.org
Other
86 stars 35 forks source link

Do not use value and namespaces to sort directives #44

Closed cedk closed 3 years ago

cedk commented 3 years ago

The value and namespaces are dictionaries that can not be ordered.

The sorted was added in d48d93c09747c130e6e2ccd5ebefe23281c4e917

FelixSchwarz commented 3 years ago

Is there a way to trigger the problem? As far as I know all tests are passing. Is this is a problem for Tryton? (sorry, I misread you profile and thought of https://github.com/naftaliharris/tauthon/).

cedk commented 3 years ago

For some reason when using i18n.Translator filter, some directives like <for each=""><for each=""> ends up in the same SUB. I do not know if it is normal or not but in this case the two same indexed directives are sorted and so the comparison is done on the other element. Indeed I think the sort should be done only on the index.

cedk commented 3 years ago

On a second though I do not really understand why d48d93c09747c130e6e2ccd5ebefe23281c4e917 added such sorting. For me the parsing order should be always respected.

FelixSchwarz commented 3 years ago

Well, that commit is ancient history and I guess by now not even cmlenz would be able to recall the rationale. That's why I'm asking for any kind of test case which provides much more context. Personally I never touched this part of the code so I'm really out of my depth here but I hope a test case makes it easier to understand why your change is right.

cedk commented 3 years ago

I added a test case.

FelixSchwarz commented 3 years ago

Thank you very much for the test case which clearly shows that there is a problem. I think we should merge this (with all 3 commits squashed?).

@hodgestar any objections? (btw: if you merge this would you mind to use rebase merging? I prefer not to use merge commits unless there is a lot of parallel development ;-)

cedk commented 3 years ago

For me it is just fixing the surface. I think there is a problem with Translator which in some way change the order of directives by grouping them inside the same directives list.

hodgestar commented 3 years ago

The directives are sorted to ensure that they are applied in a predictable order.

In Python 2, most built-in objects were comparable, so sorting by the directive index and then everything else was probably a sensible idea. In Python 3, many objects that were comparable are no longer, so sorting by arbitrary dicts, lists, etc is now a bug that wasn't picked up when we ported Genshi to Python 3.

hodgestar commented 3 years ago

@hodgestar any objections? (btw: if you merge this would you mind to use rebase merging? I prefer not to use merge commits unless there is a lot of parallel development ;-)

This is probably a discussion for the mailing list rather than this PR. Our personal preferences seem to differ, but I have no real objection to doing other kinds of merges if it's generally appealing or particularly important to people. My preference order is probably: merge commit (single commit on the target branch & keeps commit hashes), squash commit (single commit & makes target branch tidy), rebase commits (many commits, avoids merges).

cedk commented 3 years ago

But I still think the real problem is the need to sort the directives. Normally they should just have the same order as when they are parsed. As far as I can see in the repository history this sorting was cherry-picked for a i18n branch which probably introduced the Translator. So for me we should not need to sort the directives if the Translator was not grouping them in the same list of directives.

hodgestar commented 3 years ago

@cedk The element attributes in XML are explicitly unordered -- from https://www.w3.org/TR/2006/REC-xml11-20060816/#IDA10FS:

Note that the order of attribute specifications in a start-tag or empty-element tag is not significant.

Many directives in templates are associated with tags, which do have a parsing order, but others are not (e.g. <div py:match="..." py:attrs="...">).

cedk commented 3 years ago

The test case show the problem appears not with multiple attribute on the same tag but by having different tag directives merged into the same directive list.