Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905803002:
Sort all fields in alphabetical order to make the diffs deterministic (Closed)
DescriptionSort all fields in alphabetical order to make the diffs deterministic
Currently the jinja template generates the properties getters/setters
in the order they appear in the group. This means changing groups will
change the location of the getter/setter making the diff generated hard
to read.
See diff here for example: https://codereview.chromium.org/2889323002
This CL sorts fields and subgroups deterministically in alphabetical
order.
Diff: https://gist.github.com/7b50d7f151021057eded58e5a405e724/revisions
BUG=710938
Review-Url: https://codereview.chromium.org/2905803002
Cr-Commit-Position: refs/heads/master@{#474896}
Committed: https://chromium.googlesource.com/chromium/src/+/09743e506fb47d8ecc66a8bc72c83fb5635c2e21
Patch Set 1 #Patch Set 2 : no groups #Patch Set 3 : Sort fields in tmpl files #
Dependent Patchsets: Messages
Total messages: 22 (15 generated)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Sort all fields/subgroups in alphabetical order to make the diffs deterministic Currently the jinja template generates the properties getters/setters in the order they appear in the group. This means changing groups will change the location of the getter/setter making the diff generated hard to read. See here for example: https://codereview.chromium.org/2889323002 This CL sorts fields and subgroups deterministically in alphabetical order. BUG=710938 ========== to ========== Sort all fields in alphabetical order to make the diffs deterministic Currently the jinja template generates the properties getters/setters in the order they appear in the group. This means changing groups will change the location of the getter/setter making the diff generated hard to read. See diff here for example: https://codereview.chromium.org/2889323002 This CL sorts fields and subgroups deterministically in alphabetical order. BUG=710938 ==========
nainar@chromium.org changed reviewers: + shend@chromium.org
shend@, PTAL? Had to split the line in two cause presubmit. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/25 at 08:57:21, nainar wrote:
> shend@,
>
> PTAL?
> Had to split the line in two cause presubmit.
>
> Thanks!
I would keep fields sorted in their current order (by size) and change the Jinja
template to sort only the getters/setters, something like:
{% for field in group.all_fields|sort(attribute='name') %}
in both ComputedStyleBase.tmpl and group.tmpl.
On 2017/05/25 at 22:23:26, shend wrote:
> On 2017/05/25 at 08:57:21, nainar wrote:
> > shend@,
> >
> > PTAL?
> > Had to split the line in two cause presubmit.
> >
> > Thanks!
>
> I would keep fields sorted in their current order (by size) and change the
Jinja template to sort only the getters/setters, something like:
>
> {% for field in group.all_fields|sort(attribute='name') %}
>
> in both ComputedStyleBase.tmpl and group.tmpl.
Also, could you attach a diff of generated files? Thanks
Description was changed from ========== Sort all fields in alphabetical order to make the diffs deterministic Currently the jinja template generates the properties getters/setters in the order they appear in the group. This means changing groups will change the location of the getter/setter making the diff generated hard to read. See diff here for example: https://codereview.chromium.org/2889323002 This CL sorts fields and subgroups deterministically in alphabetical order. BUG=710938 ========== to ========== Sort all fields in alphabetical order to make the diffs deterministic Currently the jinja template generates the properties getters/setters in the order they appear in the group. This means changing groups will change the location of the getter/setter making the diff generated hard to read. See diff here for example: https://codereview.chromium.org/2889323002 This CL sorts fields and subgroups deterministically in alphabetical order. Diff: https://gist.github.com/7b50d7f151021057eded58e5a405e724/revisions BUG=710938 ==========
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
shend@, PTAL? Thanks!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by nainar@chromium.org
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495758917994740,
"parent_rev": "42b72063edb66bfd015bd9dea116c3e3e937d3eb", "commit_rev":
"09743e506fb47d8ecc66a8bc72c83fb5635c2e21"}
Message was sent while issue was closed.
Description was changed from ========== Sort all fields in alphabetical order to make the diffs deterministic Currently the jinja template generates the properties getters/setters in the order they appear in the group. This means changing groups will change the location of the getter/setter making the diff generated hard to read. See diff here for example: https://codereview.chromium.org/2889323002 This CL sorts fields and subgroups deterministically in alphabetical order. Diff: https://gist.github.com/7b50d7f151021057eded58e5a405e724/revisions BUG=710938 ========== to ========== Sort all fields in alphabetical order to make the diffs deterministic Currently the jinja template generates the properties getters/setters in the order they appear in the group. This means changing groups will change the location of the getter/setter making the diff generated hard to read. See diff here for example: https://codereview.chromium.org/2889323002 This CL sorts fields and subgroups deterministically in alphabetical order. Diff: https://gist.github.com/7b50d7f151021057eded58e5a405e724/revisions BUG=710938 Review-Url: https://codereview.chromium.org/2905803002 Cr-Commit-Position: refs/heads/master@{#474896} Committed: https://chromium.googlesource.com/chromium/src/+/09743e506fb47d8ecc66a8bc72c8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/09743e506fb47d8ecc66a8bc72c8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
