Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort attributes in HTML nodes when rendering #346

Merged
merged 6 commits into from
Jun 6, 2021
Merged

Conversation

MaxDesiatov
Copy link
Collaborator

@MaxDesiatov MaxDesiatov commented Jan 2, 2021

This makes attributes order deterministic and allows testing against HTML renderer output, while currently attributes order is random.

Benchmarks results:

name                            time            std        iterations
---------------------------------------------------------------------
render Text                         9667.000 ns ±   4.35 %     145213
render App unsorted attributes     51917.000 ns ±   4.23 %      26835
render App sorted attributes       52375.000 ns ±   1.62 %      26612
render List unsorted attributes 34546833.500 ns ±   0.79 %         40
render List sorted attributes   34620000.500 ns ±   0.69 %         40

Looks like on average there's ~0.2% difference in performance. I was leaning towards enabling sorting by default, but we're benchmarking here only with short attribute dictionaries, I wonder if the difference could become prominent for elements with more attributes. I kept sorting disabled by default after all, but still configurable.

var html: String on StaticHTMLRenderer was changed to func render(shouldSortAttributes: Bool = false) -> String to allow configuring this directly.

This makes attributes order deterministic and allows testing against HTML renderer output, while currently attributes order is random.
@MaxDesiatov MaxDesiatov added the test suite Changes to the framework's test suite label Jan 2, 2021
@MaxDesiatov MaxDesiatov marked this pull request as ready for review January 13, 2021 14:44
@MaxDesiatov MaxDesiatov requested a review from a team January 13, 2021 14:44
carson-katri
carson-katri previously approved these changes Jan 17, 2021
@MaxDesiatov
Copy link
Collaborator Author

I'm going to keep this open, I want to make this configurable on the renderer, so that it doesn't have any performance impact.

@MaxDesiatov MaxDesiatov marked this pull request as draft March 23, 2021 15:15
@MaxDesiatov MaxDesiatov marked this pull request as ready for review June 6, 2021 17:42
@MaxDesiatov MaxDesiatov merged commit 4428084 into main Jun 6, 2021
@MaxDesiatov MaxDesiatov deleted the ordered-attributes branch June 6, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test suite Changes to the framework's test suite
Development

Successfully merging this pull request may close these issues.

2 participants