|
|
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... |