tools/extra_imports/strict_enum_value_checker.py - Issue 170233008: Add presubmit check and automatic update script for ExtensionPermission enum.

Unified Diff: tools/extra_imports/strict_enum_value_checker.py

Issue 170233008: Add presubmit check and automatic update script for ExtensionPermission enum. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 6 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« extensions/common/permissions/PRESUBMIT.py ('K') | « tools/extra_imports/mock_enum.h ('k') | tools/extra_imports/strict_enum_value_checker_test.py » ('j') | tools/extra_imports/strict_enum_value_checker_test.py » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: tools/extra_imports/strict_enum_value_checker.py

diff --git a/chrome/browser/extensions/PRESUBMIT.py b/tools/extra_imports/strict_enum_value_checker.py

similarity index 57%

copy from chrome/browser/extensions/PRESUBMIT.py

copy to tools/extra_imports/strict_enum_value_checker.py

index c8c16b5d2987e6c2559c5680eed813189ab0bbe5..56804c28dd538dfbce1779514a42fbb73dba7a1b 100644

--- a/chrome/browser/extensions/PRESUBMIT.py

+++ b/tools/extra_imports/strict_enum_value_checker.py

@@ -1,43 +1,30 @@

rpaquay 2014/02/18 21:39:26 nit: remove the "(c"): see http://www.chromium.org

# Use of this source code is governed by a BSD-style license that can be

# found in the LICENSE file.

-"""Chromium presubmit script for src/chrome/browser/extensions.

+class StrictEnumValueChecker(object):

+ '''Verify that changes to enums are valid.

-See http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts

-for more details on the presubmit API built into gcl.

-"""

+ This class is used to check enums where reordering or deletion is not allowed,

+ and additions must be at the end of the enum, just prior to some "boundary"

+ entry. See comments at the top of the "extension_function_histogram_value.h"

+ file in chrome/browser/extensions for an example what are considered valid

+ changes. There are situations where this class gives false positive warnings,

+ i.e. it warns even though the edit is legitimate. Since the class warns using

+ prompt warnings, the user can always choose to continue. The main point is to

+ attract the attention to all (potentially or not) invalid edits.

-def GetPreferredTrySlaves():

- return ['linux_chromeos']

-class HistogramValueChecker(object):

- """Verify that changes to "extension_function_histogram_value.h" are valid.

- See comments at the top of the "extension_function_histogram_value.h" file

- for what are considered valid changes. There are situations where this script

- gives false positive warnings, i.e. it warns even though the edit is

- legitimate. Since the script warns using prompt warnings, the user can always

- choose to continue. The main point is to attract the attention to all

- (potentially or not) invalid edits.

- """

- # The name of the file we want to check against

- LOCAL_PATH = "chrome/browser/extensions/extension_function_histogram_value.h"

- # The markers we look for in the above source file as delimiters of the enum

- # definition.

- ENUM_START_MARKER = "enum HistogramValue {"

- ENUM_END_MARKER = " ENUM_BOUNDARY"

- def __init__(self, input_api, output_api):

+ '''

+ def __init__(self, input_api, output_api, start_marker, end_marker, path):

self.input_api = input_api

self.output_api = output_api

+ self.start_marker = start_marker

+ self.end_marker = end_marker

+ self.path = path

self.results = []

class EnumRange(object):

- """Represents a range of line numbers (1-based)"""

+ '''Represents a range of line numbers (1-based)'''

rpaquay 2014/02/18 21:39:26 nit: is there a particular reason you use single q

ahernandez 2014/02/20 17:23:42 There really wasn't a reason other than a personal

def __init__(self, first_line, last_line):

self.first_line = first_line

self.last_line = last_line

@@ -57,94 +44,94 @@ class HistogramValueChecker(object):

return

def ComputeEnumRangeInContents(self, contents):

- """Returns an |EnumRange| object representing the line extent of the

+ '''Returns an |EnumRange| object representing the line extent of the

HistogramValue enum members in |contents|. The line numbers are 1-based,

compatible with line numbers returned by AffectedFile.ChangeContents().

|contents| is a list of strings reprenting the lines of a text file.

- If either ENUM_START_MARKER or ENUM_END_MARKER cannot be found in

+ If either start_marker or end_marker cannot be found in

|contents|, returns None and emits detailed warnings about the problem.

- """

+ '''

first_enum_line = 0

last_enum_line = 0

line_num = 1 # Line numbers are 1-based

for line in contents:

- if line.startswith(self.ENUM_START_MARKER):

+ if line.startswith(self.start_marker):

first_enum_line = line_num + 1

- elif line.startswith(self.ENUM_END_MARKER):

+ elif line.startswith(self.end_marker):

last_enum_line = line_num

line_num += 1

if first_enum_line == 0:

- self.EmitWarning("The presubmit script could not find the start of the "

- "enum definition (\"%s\"). Did the enum definition "

- "change?" % self.ENUM_START_MARKER)

+ self.EmitWarning('The presubmit script could not find the start of the '

+ 'enum definition ("%s"). Did the enum definition '

+ 'change?' % self.start_marker)

return None

if last_enum_line == 0:

- self.EmitWarning("The presubmit script could not find the end of the "

- "enum definition (\"%s\"). Did the enum definition "

- "change?" % self.ENUM_END_MARKER)

+ self.EmitWarning('The presubmit script could not find the end of the '

+ 'enum definition ("%s"). Did the enum definition '

+ 'change?' % self.end_marker)

return None

if first_enum_line >= last_enum_line:

- self.EmitWarning("The presubmit script located the start of the enum "

- "definition (\"%s\" at line %d) *after* its end "

- "(\"%s\" at line %d). Something is not quite right."

- % (self.ENUM_START_MARKER, first_enum_line,

- self.ENUM_END_MARKER, last_enum_line))

+ self.EmitWarning('The presubmit script located the start of the enum '

+ 'definition ("%s" at line %d) *after* its end '

+ '("%s" at line %d). Something is not quite right.'

+ % (self.start_marker, first_enum_line,

+ self.end_marker, last_enum_line))

return None

- self.LogInfo("Line extent of |HistogramValue| enum definition: "

- "first_line=%d, last_line=%d."

+ self.LogInfo('Line extent of |HistogramValue| enum definition: '

+ 'first_line=%d, last_line=%d.'

% (first_enum_line, last_enum_line))

return self.EnumRange(first_enum_line, last_enum_line)

def ComputeEnumRangeInNewFile(self, affected_file):

return self.ComputeEnumRangeInContents(affected_file.NewContents())

- def GetLongMessage(self):

- return str("The file \"%s\" contains the definition of the "

- "|HistogramValue| enum which should be edited in specific ways "

rpaquay 2014/02/18 21:39:26 "HistogramValue" is too specific in this file, as

- "only - *** read the comments at the top of the header file ***"

- ". There are changes to the file that may be incorrect and "

- "warrant manual confirmation after review. Note that this "

- "presubmit script can not reliably report the nature of all "

- "types of invalid changes, especially when the diffs are "

- "complex. For example, an invalid deletion may be reported "

- "whereas the change contains a valid rename."

- % self.LOCAL_PATH)

+ def GetLongMessage(self, local_path):

+ return str('The file "%s" contains the definition of the '

+ '|HistogramValue| enum which should be edited in specific ways '

+ 'only - *** read the comments at the top of the header file ***'

+ '. There are changes to the file that may be incorrect and '

+ 'warrant manual confirmation after review. Note that this '

+ 'presubmit script can not reliably report the nature of all '

+ 'types of invalid changes, especially when the diffs are '

+ 'complex. For example, an invalid deletion may be reported '

+ 'whereas the change contains a valid rename.'

+ % local_path)

def EmitWarning(self, message, line_number=None, line_text=None):

- """Emits a presubmit prompt warning containing the short message

+ '''Emits a presubmit prompt warning containing the short message

|line_text|.

- """

+ '''

if line_number is not None and line_text is not None:

- item = "%s(%d): %s" % (self.LOCAL_PATH, line_number, line_text)

+ item = '%s(%d): %s' % (self.path, line_number, line_text)

elif line_number is not None:

- item = "%s(%d)" % (self.LOCAL_PATH, line_number)

+ item = '%s(%d)' % (self.path, line_number)

else:

- item = self.LOCAL_PATH

- long_message = self.GetLongMessage()

+ item = self.path

+ long_message = self.GetLongMessage(self.path)

self.LogInfo(message)

self.results.append(

self.output_api.PresubmitPromptWarning(message, [item], long_message))

def CollectRangesInsideEnumDefinition(self, affected_file,

first_line, last_line):

- """Returns a list of triplet (line_start, line_end, line_text) of ranges of

+ '''Returns a list of triplet (line_start, line_end, line_text) of ranges of

edits changes. The |line_text| part is the text at line |line_start|.

Since it used only for reporting purposes, we do not need all the text

lines in the range.

- """

+ '''

results = []

previous_line_number = 0

previous_range_start_line_number = 0

- previous_range_start_text = ""

+ previous_range_start_text = ''

def addRange():

tuple = (previous_range_start_line_number,

@@ -154,7 +141,7 @@ class HistogramValueChecker(object):

for line_number, line_text in affected_file.ChangedContents():

if first_line <= line_number and line_number <= last_line:

- self.LogDebug("Line change at line number " + str(line_number) + ": " +

+ self.LogDebug('Line change at line number ' + str(line_number) + ': ' +

line_text)

# Start a new interval if none started

if previous_range_start_line_number == 0:

@@ -173,17 +160,17 @@ class HistogramValueChecker(object):

return results

def CheckForFileDeletion(self, affected_file):

- """Emits a warning notification if file has been deleted """

+ '''Emits a warning notification if file has been deleted '''

if not affected_file.NewContents():

- self.EmitWarning("The file seems to be deleted in the changelist. If "

- "your intent is to really delete the file, the code in "

- "PRESUBMIT.py should be updated to remove the "

- "|HistogramValueChecker| class.");

rpaquay 2014/02/18 21:39:26 nit: HistogramValueChecker is too specific.

+ self.EmitWarning('The file seems to be deleted in the changelist. If '

+ 'your intent is to really delete the file, the code in '

+ 'PRESUBMIT.py should be updated to remove the '

+ '|HistogramValueChecker| class.');

return False

return True

def GetDeletedLinesFromScmDiff(self, affected_file):

- """Return a list of of line numbers (1-based) corresponding to lines

+ '''Return a list of of line numbers (1-based) corresponding to lines

deleted from the new source file (if they had been present in it). Note

that if multiple contiguous lines have been deleted, the returned list will

contain contiguous line number entries. To prevent false positives, we

@@ -194,7 +181,7 @@ class HistogramValueChecker(object):

content nor the list of "delete" changes from the current presubmit script

API.

- """

+ '''

results = []

line_num = 0

deleting_lines = False

@@ -220,11 +207,11 @@ class HistogramValueChecker(object):

return results

def CheckForEnumEntryDeletions(self, affected_file):

- """Look for deletions inside the enum definition. We currently use a

+ '''Look for deletions inside the enum definition. We currently use a

simple heuristics (not 100% accurate): if there are deleted lines inside

the enum definition, this might be a deletion.

- """

+ '''

range_new = self.ComputeEnumRangeInNewFile(affected_file)

if not range_new:

return False

@@ -232,8 +219,8 @@ class HistogramValueChecker(object):

is_ok = True

for line_num in self.GetDeletedLinesFromScmDiff(affected_file):

if range_new.Contains(line_num):

- self.EmitWarning("It looks like you are deleting line(s) from the "

- "enum definition. This should never happen.",

+ self.EmitWarning('It looks like you are deleting line(s) from the '

+ 'enum definition. This should never happen.',

line_num)

is_ok = False

return is_ok

@@ -260,18 +247,18 @@ class HistogramValueChecker(object):

# vs invalid insertions, so we sometimes will warn for valid edits.

is_valid_edit = (line_end == last_line - 1)

- self.LogDebug("Edit range in new file at starting at line number %d and "

- "ending at line number %d: valid=%s"

+ self.LogDebug('Edit range in new file at starting at line number %d and '

+ 'ending at line number %d: valid=%s'

% (line_start, line_end, is_valid_edit))

if not is_valid_edit:

- self.EmitWarning("The change starting at line %d and ending at line "

- "%d is *not* located *exactly* at the end of the "

- "enum definition. Unless you are renaming an "

- "existing entry, this is not a valid changes, as new "

- "entries should *always* be added at the end of the "

- "enum definition, right before the 'ENUM_BOUNDARY' "

- "entry." % (line_start, line_end),

+ self.EmitWarning('The change starting at line %d and ending at line '

+ '%d is *not* located *exactly* at the end of the '

+ 'enum definition. Unless you are renaming an '

+ 'existing entry, this is not a valid changes, as new '

+ 'entries should *always* be added at the end of the '

+ 'enum definition, right before the "ENUM_BOUNDARY" '

+ 'entry.' % (line_start, line_end),

line_start,

line_text)

is_ok = False

@@ -286,19 +273,12 @@ class HistogramValueChecker(object):

return

def ProcessHistogramValueFile(self, affected_file):

- self.LogInfo("Start processing file \"%s\"" % affected_file.LocalPath())

+ self.LogInfo('Start processing file "%s"' % affected_file.LocalPath())

self.PerformChecks(affected_file)

- self.LogInfo("Done processing file \"%s\"" % affected_file.LocalPath())

+ self.LogInfo('Done processing file "%s"' % affected_file.LocalPath())

def Run(self):

for file in self.input_api.AffectedFiles(include_deletes=True):

- if file.LocalPath() == self.LOCAL_PATH:

+ if file.LocalPath() == self.path:

self.ProcessHistogramValueFile(file)

return self.results

-def CheckChangeOnUpload(input_api, output_api):

- results = []

- results += HistogramValueChecker(input_api, output_api).Run()

- results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api)

- return results