Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2850813003:
Add presubmit script checking FlatBuffer changes. (Closed)
|
Created:
3 years, 7 months ago by pkalinnikov Modified:
3 years, 7 months ago CC:
chromium-reviews, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd presubmit script checking FlatBuffer changes.
This change introduces a presubmit check ensuring that developers modifying
.fbs files in subresource_filter get reminded to change IndexedRuleset format
version (if that is required).
BUG=609747
Review-Url: https://codereview.chromium.org/2850813003
Cr-Commit-Position: refs/heads/master@{#468618}
Committed: https://chromium.googlesource.com/chromium/src/+/e27f9842f0a01769ba463a45174390bc70807139
Patch Set 1 : Add presubmit script checking FlatBuffer changes. #
Total comments: 21
Patch Set 2 : Address comments. #
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by pkalinnikov@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 pkalinnikov@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...
Patchset #1 (id:1) has been deleted
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
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 ========== Add presubmit script checking FlatBuffer changes. BUG= ========== to ========== Add presubmit script checking FlatBuffer changes. BUG=609747 ==========
Description was changed from ========== Add presubmit script checking FlatBuffer changes. BUG=609747 ========== to ========== Add presubmit script checking FlatBuffer changes. This change introduces a presubmit check ensuring that developers modifying .fbs files in subresource_filter get reminded to change IndexedRuleset format version (if that is required). BUG=609747 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkalinnikov@chromium.org changed reviewers: + csharrison@chromium.org, engedy@chromium.org
PTAL. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... File components/subresource_filter/core/common/PRESUBMIT.py (right): https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): Probably also need to check the file's directory. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:25: if 'kIndexedFormatVersion = ' in line: Do we need to comment the corresponding line in indexed_ruleset.cc with a link to this script and explanation why it's needed? https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:25: if 'kIndexedFormatVersion = ' in line: nit: RulesetIndexer::kIndexedFormatVersion?
LGTM % comments. Thanks a lot for adding this! https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... File components/subresource_filter/core/common/PRESUBMIT.py (right): https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:12: """Checks that IndexedRuleset format version is updated if the FlatBuffers nit: The style guide recommend a one-line summary followed by a blank line, and then an arbitrarily long description: """Foo. Bla bla bla.""" https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:20: basename = input_api.os_path.basename(affected_file.LocalPath()) nit: I think there is a shortcut: input_api.basename(...) https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): If I remember correctly, we already had one or two changes where the Flatbuffers IDL was not changed, yet we increased the version number. Can we catch those cases somehow as well? Maybe watch indexed_ruleset.cc for this purpose as well? Or check every changed file that includes the generate header from flatbuffers? https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:25: if 'kIndexedFormatVersion = ' in line: On 2017/04/28 16:35:30, pkalinnikov wrote: > Do we need to comment the corresponding line in indexed_ruleset.cc with a link > to this script and explanation why it's needed? Yeah, I think that would be great. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:25: if 'kIndexedFormatVersion = ' in line: nit: Lets use "kIndexedFormatVersion =", in case somebody does some refactoring and there is a line break after the '='. (I'm hoping here that ChangedContents also includes diff context. :S) https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:38: def CheckChangeOnCommit(input_api, output_api): nit: Will anyone see this warning?
LGTM/2 when Balazs' comments are addressed. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... File components/subresource_filter/core/common/PRESUBMIT.py (right): https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): On 2017/04/28 17:40:48, engedy wrote: > If I remember correctly, we already had one or two changes where the Flatbuffers > IDL was not changed, yet we increased the version number. Can we catch those > cases somehow as well? Maybe watch indexed_ruleset.cc for this purpose as well? > Or check every changed file that includes the generate header from flatbuffers? +1 I think watching all of indexed_ruleset is not a bad idea, as long as the message isn't too pushy.
Addressed comments. Balazs, PTAL. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... File components/subresource_filter/core/common/PRESUBMIT.py (right): https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:12: """Checks that IndexedRuleset format version is updated if the FlatBuffers On 2017/04/28 17:40:48, engedy wrote: > nit: The style guide recommend a one-line summary followed by a blank line, and > then an arbitrarily long description: > > """Foo. > > Bla > bla > bla.""" Done. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:20: basename = input_api.os_path.basename(affected_file.LocalPath()) On 2017/04/28 17:40:48, engedy wrote: > nit: I think there is a shortcut: input_api.basename(...) Done. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): On 2017/04/28 16:35:30, pkalinnikov wrote: > Probably also need to check the file's directory. Done. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): On 2017/04/28 17:40:48, engedy wrote: > If I remember correctly, we already had one or two changes where the Flatbuffers > IDL was not changed, yet we increased the version number. Can we catch those > cases somehow as well? Maybe watch indexed_ruleset.cc for this purpose as well? > Or check every changed file that includes the generate header from flatbuffers? SGTM, added indexed_ruleset.cc to the watch list. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): On 2017/05/01 16:14:06, Charlie (ooo-ish until may 2) wrote: > On 2017/04/28 17:40:48, engedy wrote: > > If I remember correctly, we already had one or two changes where the > Flatbuffers > > IDL was not changed, yet we increased the version number. Can we catch those > > cases somehow as well? Maybe watch indexed_ruleset.cc for this purpose as > well? > > Or check every changed file that includes the generate header from > flatbuffers? > > +1 I think watching all of indexed_ruleset is not a bad idea, as long as the > message isn't too pushy. Done. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:25: if 'kIndexedFormatVersion = ' in line: On 2017/04/28 16:35:30, pkalinnikov wrote: > nit: RulesetIndexer::kIndexedFormatVersion? Nah, let it be just kIndexedFormatVersion. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:25: if 'kIndexedFormatVersion = ' in line: On 2017/04/28 17:40:48, engedy wrote: > nit: Lets use "kIndexedFormatVersion =", in case somebody does some refactoring > and there is a line break after the '='. (I'm hoping here that ChangedContents > also includes diff context. :S) Done. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:25: if 'kIndexedFormatVersion = ' in line: On 2017/04/28 17:40:48, engedy wrote: > On 2017/04/28 16:35:30, pkalinnikov wrote: > > Do we need to comment the corresponding line in indexed_ruleset.cc with a link > > to this script and explanation why it's needed? > > Yeah, I think that would be great. Done. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:38: def CheckChangeOnCommit(input_api, output_api): On 2017/04/28 17:40:48, engedy wrote: > nit: Will anyone see this warning? I am not sure when exactly it is run and on which machine (local or bots). Removed it.
LGTM % one small request. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... File components/subresource_filter/core/common/PRESUBMIT.py (right): https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): On 2017/05/02 11:13:03, pkalinnikov wrote: > On 2017/04/28 17:40:48, engedy wrote: > > If I remember correctly, we already had one or two changes where the > Flatbuffers > > IDL was not changed, yet we increased the version number. Can we catch those > > cases somehow as well? Maybe watch indexed_ruleset.cc for this purpose as > well? > > Or check every changed file that includes the generate header from > flatbuffers? > > SGTM, added indexed_ruleset.cc to the watch list. Let's add "url_pattern_index.cc" as well either here or in https://codereview.chromium.org/2844293003/.
Thanks! The request will be fulfilled in crrev/2844293003. Submitting, hope Charlie won't have objections. https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... File components/subresource_filter/core/common/PRESUBMIT.py (right): https://codereview.chromium.org/2850813003/diff/40001/components/subresource_... components/subresource_filter/core/common/PRESUBMIT.py:21: if basename.endswith('.fbs'): On 2017/05/02 11:15:40, engedy wrote: > On 2017/05/02 11:13:03, pkalinnikov wrote: > > On 2017/04/28 17:40:48, engedy wrote: > > > If I remember correctly, we already had one or two changes where the > > Flatbuffers > > > IDL was not changed, yet we increased the version number. Can we catch those > > > cases somehow as well? Maybe watch indexed_ruleset.cc for this purpose as > > well? > > > Or check every changed file that includes the generate header from > > flatbuffers? > > > > SGTM, added indexed_ruleset.cc to the watch list. > > Let's add "url_pattern_index.cc" as well either here or in > https://codereview.chromium.org/2844293003/. Sure. I will do it in the linked CL, because I planned to put it on top of this one.
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2850813003/#ps60001 (title: "Address comments.")
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": 60001, "attempt_start_ts": 1493729271650750,
"parent_rev": "2091c3c5224ab8907aab48719ea6c967283388b4", "commit_rev":
"e27f9842f0a01769ba463a45174390bc70807139"}
Message was sent while issue was closed.
Description was changed from ========== Add presubmit script checking FlatBuffer changes. This change introduces a presubmit check ensuring that developers modifying .fbs files in subresource_filter get reminded to change IndexedRuleset format version (if that is required). BUG=609747 ========== to ========== Add presubmit script checking FlatBuffer changes. This change introduces a presubmit check ensuring that developers modifying .fbs files in subresource_filter get reminded to change IndexedRuleset format version (if that is required). BUG=609747 Review-Url: https://codereview.chromium.org/2850813003 Cr-Commit-Position: refs/heads/master@{#468618} Committed: https://chromium.googlesource.com/chromium/src/+/e27f9842f0a01769ba463a451743... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e27f9842f0a01769ba463a451743... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
