Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714773003:
Extension Docs: Correct how enum values are rendered. (Closed)
|
Created:
3 years, 10 months ago by karandeepb Modified:
3 years, 9 months ago Reviewers:
lazyboy CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCurrently, enums with only a single possible value are prepended with a
redundant "or" in the extension documentation. This CL modifies the template
rendering the enums, to prevent this.
BUG=695713
Review-Url: https://codereview.chromium.org/2714773003
Cr-Commit-Position: refs/heads/master@{#456812}
Committed: https://chromium.googlesource.com/chromium/src/+/8d51c791c97394ab44a0c8376e8ef95c55fbee98
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add first to enum template parameters #
Total comments: 6
Patch Set 3 : Address review. #Patch Set 4 : Rebase to fix presubmit. #Patch Set 5 : Bump versions #Patch Set 6 : Rebase #
Depends on Patchset: Messages
Total messages: 34 (23 generated)
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Extension Docs: Correct how enum values are rendered. ========== to ========== Currently, enums with only a single possible value are prepended with a redundant "or". This CL modified the templated rendering the enums, to prevent this. BUG=695713 ==========
Description was changed from ========== Currently, enums with only a single possible value are prepended with a redundant "or". This CL modified the templated rendering the enums, to prevent this. BUG=695713 ========== to ========== Currently, enums with only a single possible value are prepended with a redundant "or" in the extension documentation. This CL modified the template rendering the enums, to prevent this. BUG=695713 ==========
karandeepb@chromium.org changed reviewers: + lazyboy@chromium.org
PTAL Istiaque. Currently, the last enum value is prepended with a "or". This CL removes it to solve the bug. I didn't find a way in the templating language to query the list length, so if we want to show the "or", we can pass additional data to the template. Also, I think the presubmit failures are unrelated to my change. Even adding an empty line to the file causes failures. Will file a bug for it.
https://codereview.chromium.org/2714773003/diff/1/chrome/common/extensions/do... File chrome/common/extensions/docs/templates/private/type.html (left): https://codereview.chromium.org/2714773003/diff/1/chrome/common/extensions/do... chrome/common/extensions/docs/templates/private/type.html:25: {{?e.last}}or {{/}}<code>"{{e.name}}"</code>{{^e.last}}, {{/}} I think you can use e.first, sth like this (to get an idea, haven't thoroughly double checked myself) {{^e.first}} {{?e.last}} or {{/e.last}}{{^e.last}}, {{/e.last}} {{/e.first}} <code>{{e.name}}</code>
PTAL Istiaque. https://codereview.chromium.org/2714773003/diff/1/chrome/common/extensions/do... File chrome/common/extensions/docs/templates/private/type.html (left): https://codereview.chromium.org/2714773003/diff/1/chrome/common/extensions/do... chrome/common/extensions/docs/templates/private/type.html:25: {{?e.last}}or {{/}}<code>"{{e.name}}"</code>{{^e.last}}, {{/}} On 2017/02/25 00:51:37, lazyboy wrote: > I think you can use e.first, sth like this (to get an idea, haven't thoroughly > double checked myself) > {{^e.first}} > {{?e.last}} or {{/e.last}}{{^e.last}}, {{/e.last}} > {{/e.first}} > <code>{{e.name}}</code> Introduced e.first as a template parameter. Also corrected this at another place.
One question about space https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/private/type.html (right): https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/private/type.html:25: {{^e.last}} nit: 2 space indentation Ah, I assumed we already populated first :( https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/private/type.html:26: <code>"{{e.name}}"</code>, We don't have explicit spaces after comma anymore, I'm guessing that turns out OK in html?
Description was changed from ========== Currently, enums with only a single possible value are prepended with a redundant "or" in the extension documentation. This CL modified the template rendering the enums, to prevent this. BUG=695713 ========== to ========== Currently, enums with only a single possible value are prepended with a redundant "or" in the extension documentation. This CL modifies the template rendering the enums, to prevent this. BUG=695713 ==========
PTAL Istiaque. https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/private/type.html (right): https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/private/type.html:25: {{^e.last}} On 2017/02/25 03:48:05, lazyboy wrote: > nit: 2 space indentation > > Ah, I assumed we already populated first :( Done. Thought git cl format should take care of these things. https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/private/type.html:26: <code>"{{e.name}}"</code>, On 2017/02/25 03:48:05, lazyboy wrote: > We don't have explicit spaces after comma anymore, I'm guessing that turns out > OK in html? Yeah it does, since the template renderer introduces extra whitespace. Nevertheless, added an explicit space.
lgtm https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/private/type.html (right): https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/private/type.html:25: {{^e.last}} On 2017/02/27 03:06:20, karandeepb wrote: > On 2017/02/25 03:48:05, lazyboy wrote: > > nit: 2 space indentation > > > > Ah, I assumed we already populated first :( > > Done. Thought git cl format should take care of these things. I think git cl format only works for cpp. That's probably why it didn't work.
https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/private/type.html (right): https://codereview.chromium.org/2714773003/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/private/type.html:25: {{^e.last}} On 2017/02/27 17:54:23, lazyboy wrote: > On 2017/02/27 03:06:20, karandeepb wrote: > > On 2017/02/25 03:48:05, lazyboy wrote: > > > nit: 2 space indentation > > > > > > Ah, I assumed we already populated first :( > > > > Done. Thought git cl format should take care of these things. > > I think git cl format only works for cpp. That's probably why it didn't work. Think it also works for gn files.
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2714773003/#ps120001 (title: "Rebase")
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": 120001, "attempt_start_ts": 1489522209125210,
"parent_rev": "bb4a1867005a202b2b5f4245fc543e66109a8f14", "commit_rev":
"8d51c791c97394ab44a0c8376e8ef95c55fbee98"}
Message was sent while issue was closed.
Description was changed from ========== Currently, enums with only a single possible value are prepended with a redundant "or" in the extension documentation. This CL modifies the template rendering the enums, to prevent this. BUG=695713 ========== to ========== Currently, enums with only a single possible value are prepended with a redundant "or" in the extension documentation. This CL modifies the template rendering the enums, to prevent this. BUG=695713 Review-Url: https://codereview.chromium.org/2714773003 Cr-Commit-Position: refs/heads/master@{#456812} Committed: https://chromium.googlesource.com/chromium/src/+/8d51c791c97394ab44a0c8376e8e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8d51c791c97394ab44a0c8376e8e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
