Syntax checker for policy_templates.json

Syntax checker for policy_templates.json

To make sure that all policy definitions match the expected format. The checker is called as a presubmit hook, and can also be invoked manually. Run it without any parameters (or with --help) to get usage information.


BUG=69527
TEST=manual: checker reports no errors for correct policy definitions, checker complains for malformed policy definitions, checker is invoked as presubmit hook when policy_templates.json has been changed

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71778

[Jakob Kummerow, 2011-01-13 10:56:30]

Here you go. 
As discussed, this depends on the following pending changes to the .json file
format:
- split type 'enum' into 'int-enum'/'string-enum'
- support type 'int'
- include 'desc'/'caption' in policy definitions
- remove 'annotations' dict and include its former contents directly

[gfeher, 2011-01-14 11:57:26]

Looks cool!
Please add one-liner docstring comments to each non-trivial method. It would
help me better understing your code.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:15: 
Please insert newline.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:31:
offending_snippet=None):
Please fix indenting.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:56: Offending snippet:
|container[key]|
It would help understandability of your code if you'd add more comments here.
E.g.: return value is None when the key is not found, three words for each
important argument.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:112:
self._CheckContains(policy, 'caption', str)
Groups should also have captions and descriptions.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:321: parent_element=None,
container_name='The root element', offending=None)
Please take a look at (and fix indenting here):
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Indentation

[Jakob Kummerow, 2011-01-14 12:38:52]

Thanks for the comments.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:15: 
On 2011/01/14 11:57:26, gfeher wrote:
> Please insert newline.

Done.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:31:
offending_snippet=None):
On 2011/01/14 11:57:26, gfeher wrote:
> Please fix indenting.

Done.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:56: Offending snippet:
|container[key]|
On 2011/01/14 11:57:26, gfeher wrote:
> It would help understandability of your code if you'd add more comments here.
> E.g.: return value is None when the key is not found, three words for each
> important argument.

Done.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:112:
self._CheckContains(policy, 'caption', str)
On 2011/01/14 11:57:26, gfeher wrote:
> Groups should also have captions and descriptions.

Done. Good catch.

http://codereview.chromium.org/6299001/diff/1/chrome/app/policy/syntax_check_...
chrome/app/policy/syntax_check_policy_template_json.py:321: parent_element=None,
container_name='The root element', offending=None)
On 2011/01/14 11:57:26, gfeher wrote:
> Please take a look at (and fix indenting here):
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Indentation

Done.
Same for two more occurrences below.

[gfeher, 2011-01-14 13:25:07]

No more comments on code, let's see what does this do with the JSON file when we
get there!

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:46:
offending='__CONTAINER__'):
Nit: =None is the usual pattern here.

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:56: Offending snippet:
|offending| (if specified, defaults to |container|)
"if not specified"?

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:155:
self._CheckContains(policy, 'label', str, True)
Nit/suggestion: move these two up next to 'caption' and 'desc'?

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:185: (policy.get('name'),
key))
My general suggestion is that you should move the checks that are
type-independent next to each other, e.g.: at the beginning of this method. (Or
probably a separate method, _CheckCommonPolicyKeys)

[Jakob Kummerow, 2011-01-14 14:41:55]

> No more comments on code, let's see what does this do with the JSON file when
we
> get there!

$ python syntax_check_policy_template_json.py pt_good.json 
Finished. 0 errors, 0 warnings.

:-)

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:46:
offending='__CONTAINER__'):
On 2011/01/14 13:25:07, gfeher wrote:
> Nit: =None is the usual pattern here.

Yes, and I'm using =None elsewhere, but I can't use it in this case because None
is precisely the custom value that's passed in to override the default behavior.

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:56: Offending snippet:
|offending| (if specified, defaults to |container|)
On 2011/01/14 13:25:07, gfeher wrote:
> "if not specified"?

s/,/;/ has the same effect, wouldn't you agree? The sentence is supposed to be
short for:
|offending| if |offending| is specified; otherwise defaults to |container|

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:155:
self._CheckContains(policy, 'label', str, True)
On 2011/01/14 13:25:07, gfeher wrote:
> Nit/suggestion: move these two up next to 'caption' and 'desc'?

Done.

http://codereview.chromium.org/6299001/diff/5001/chrome/app/policy/syntax_che...
chrome/app/policy/syntax_check_policy_template_json.py:185: (policy.get('name'),
key))
On 2011/01/14 13:25:07, gfeher wrote:
> My general suggestion is that you should move the checks that are
> type-independent next to each other, e.g.: at the beginning of this method.
(Or
> probably a separate method, _CheckCommonPolicyKeys)

Done.

[gfeher, 2011-01-18 10:04:07]

LGTM with nits

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:75: container_name,
identifier, offending)
Nit: indentation. Please align these two lines to 
_Error('%s must have
       ^

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:81: container_name,
identifier, value)
Nit:indentation.

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:177:
identifier=policy.get('name'))
Nit: policy['name']

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:181:
identifier=policy.get('name'))
Nit: policy['name']

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:185:
identifier=policy.get('name'))
Nit: policy['name']

[Jakob Kummerow, 2011-01-18 10:38:36]

gfeher: fixed your nits.

maruel: please take a look at chrome/app/policy/PRESUBMIT.py

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
File chrome/app/policy/syntax_check_policy_template_json.py (right):

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:75: container_name,
identifier, offending)
On 2011/01/18 10:04:07, gfeher wrote:
> Nit: indentation. Please align these two lines to 
> _Error('%s must have
>        ^

Done.

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:81: container_name,
identifier, value)
On 2011/01/18 10:04:07, gfeher wrote:
> Nit:indentation.

Done.

http://codereview.chromium.org/6299001/diff/11001/chrome/app/policy/syntax_ch...
chrome/app/policy/syntax_check_policy_template_json.py:177:
identifier=policy.get('name'))
On 2011/01/18 10:04:07, gfeher wrote:
> Nit: policy['name']

This (as well as the other two occurrences) is actually done on purpose, because
at this point in the code, I don't check whether or not |policy| contains a key
'name'. If I used "policy['name']" when 'name' isn't present, I would get an
exception (resulting in a crash since it's not caught); "policy.get('name')"
however silently returns |None| if 'name' isn't found, which is exactly what I
want in that case.
I added a comment explaining this.

[M-A Ruel, 2011-01-18 13:52:03]

http://codereview.chromium.org/6299001/diff/16001/chrome/app/policy/PRESUBMIT.py
File chrome/app/policy/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/16001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:17: if f.AbsoluteLocalPath() ==
os.path.join(input_api.PresubmitLocalPath(),
input_api.os_path.join(...

http://codereview.chromium.org/6299001/diff/16001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:19:
sys.path.append(input_api.PresubmitLocalPath())
Don't do that, it affects all following presubmit checks.

The work around I used in another PRESUBMIT.py file is to wrap that around a
try/finally. It's still loosy but it's better than this.

old_sys_path = sys.path
try:
  sys.path = [input_api.PresubmitLocalPath()] + sys.path
  import syntax_check_policy_template_json
  ...
finally:
  sys.path = old_sys_path

[Jakob Kummerow, 2011-01-18 15:02:35]

maruel: thanks for your comments. I changed both things according to your
proposals. Is there anything else that you think could be improved?

Btw, I took the offending code from ceee/PRESUBMIT.py. Would you like me to
perform the same cleanup there?

http://codereview.chromium.org/6299001/diff/16001/chrome/app/policy/PRESUBMIT.py
File chrome/app/policy/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/16001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:17: if f.AbsoluteLocalPath() ==
os.path.join(input_api.PresubmitLocalPath(),
On 2011/01/18 13:52:03, Marc-Antoine Ruel wrote:
> input_api.os_path.join(...

Done.

http://codereview.chromium.org/6299001/diff/16001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:19:
sys.path.append(input_api.PresubmitLocalPath())
On 2011/01/18 13:52:03, Marc-Antoine Ruel wrote:
> Don't do that, it affects all following presubmit checks.
> 
> The work around I used in another PRESUBMIT.py file is to wrap that around a
> try/finally. It's still loosy but it's better than this.
> 
> old_sys_path = sys.path
> try:
>   sys.path = [input_api.PresubmitLocalPath()] + sys.path
>   import syntax_check_policy_template_json
>   ...
> finally:
>   sys.path = old_sys_path

Done.

[M-A Ruel, 2011-01-18 15:10:33]

> Btw, I took the offending code from ceee/PRESUBMIT.py. Would you like me to
> perform the same cleanup there?

You can if you don't mind. It's Joi that wrote ceee/PRESUBMIT.py which uses the
invalid pattern. Also ceee_presubmit.py is quite messy, I'm not sure what reason
drove its creation.

http://codereview.chromium.org/6299001/diff/22001/chrome/app/policy/PRESUBMIT.py
File chrome/app/policy/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/22001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:19: try:
BTW, you can move the sys.path mangling outside of the loop since
input_api.PresubmitLocalPath() is invariant throughout this function. It would
make the code more readable too.

[Jakob Kummerow, 2011-01-18 15:51:28]

Here you go.

On 2011/01/18 15:10:33, Marc-Antoine Ruel wrote:
> > Btw, I took the offending code from ceee/PRESUBMIT.py. Would you like me to
> > perform the same cleanup there?
> 
> You can if you don't mind.

Done.

http://codereview.chromium.org/6299001/diff/22001/chrome/app/policy/PRESUBMIT.py
File chrome/app/policy/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/22001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:19: try:
On 2011/01/18 15:10:33, Marc-Antoine Ruel wrote:
> BTW, you can move the sys.path mangling outside of the loop since
> input_api.PresubmitLocalPath() is invariant throughout this function. It would
> make the code more readable too.

Done.

[M-A Ruel, 2011-01-18 16:06:23]

The main reason I advocate to remove the loop is that it's easy to mistakenly
think that it's about to process multiple files when it won't.

http://codereview.chromium.org/6299001/diff/27001/ceee/PRESUBMIT.py
File ceee/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/27001/ceee/PRESUBMIT.py#newcode14
ceee/PRESUBMIT.py:14: sys.path =
([input_api.os_path.join(input_api.PresubmitLocalPath(),
nit: I think you should create a named variable since it's hard to read.

http://codereview.chromium.org/6299001/diff/27001/ceee/PRESUBMIT.py#newcode18
ceee/PRESUBMIT.py:18: result = ceee_presubmit.CheckChange(input_api,
you can return here, the finally clause will still be correctly executed.

http://codereview.chromium.org/6299001/diff/27001/chrome/app/policy/PRESUBMIT.py
File chrome/app/policy/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/27001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:9: import traceback
pelase remove this import, it's not used.

http://codereview.chromium.org/6299001/diff/27001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:18: for f in input_api.AffectedFiles():
nit: Sorry for the round-trips but what about changing the loop into something
more readable (IMHO)?

filepath = input_api.os_path.join(
    input_api.PresubmitLocalPath(), 'policy_templates.json')

if any(f for f in input_api.AffectedFiles()
       if f.AbsoluteLocalPath() == filepath):
  old_sys_path = sys.path
  try:
    sys.path = [input_api.PresubmitLocalPath()] + sys.path
    import syntax_check_policy_template_json
    if checker.Run([], filepath) > 0:
      return [output_api.PresubmitError('Syntax error(s) in file(s):',
[filepath]))
  finally:
    sys.path = old_sys_path
return []

[Jakob Kummerow, 2011-01-18 16:33:54]

http://codereview.chromium.org/6299001/diff/27001/ceee/PRESUBMIT.py
File ceee/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/27001/ceee/PRESUBMIT.py#newcode14
ceee/PRESUBMIT.py:14: sys.path =
([input_api.os_path.join(input_api.PresubmitLocalPath(),
On 2011/01/18 16:06:23, Marc-Antoine Ruel wrote:
> nit: I think you should create a named variable since it's hard to read.

Done.

http://codereview.chromium.org/6299001/diff/27001/ceee/PRESUBMIT.py#newcode18
ceee/PRESUBMIT.py:18: result = ceee_presubmit.CheckChange(input_api,
On 2011/01/18 16:06:23, Marc-Antoine Ruel wrote:
> you can return here, the finally clause will still be correctly executed.

Done.

http://codereview.chromium.org/6299001/diff/27001/chrome/app/policy/PRESUBMIT.py
File chrome/app/policy/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/27001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:9: import traceback
On 2011/01/18 16:06:23, Marc-Antoine Ruel wrote:
> pelase remove this import, it's not used.

Done.
(Leftover from an earlier version. Should've noticed that.)

http://codereview.chromium.org/6299001/diff/27001/chrome/app/policy/PRESUBMIT...
chrome/app/policy/PRESUBMIT.py:18: for f in input_api.AffectedFiles():
On 2011/01/18 16:06:23, Marc-Antoine Ruel wrote:
> nit: Sorry for the round-trips 

No problem at all. I'm always happy to learn how to do things better.

> but what about changing the loop into something
> more readable (IMHO)?
> 
> filepath = input_api.os_path.join(
>     input_api.PresubmitLocalPath(), 'policy_templates.json')
> 
> if any(f for f in input_api.AffectedFiles()
>        if f.AbsoluteLocalPath() == filepath):
>   old_sys_path = sys.path
>   try:
>     sys.path = [input_api.PresubmitLocalPath()] + sys.path
>     import syntax_check_policy_template_json
>     if checker.Run([], filepath) > 0:
>       return [output_api.PresubmitError('Syntax error(s) in file(s):',
> [filepath]))
>   finally:
>     sys.path = old_sys_path
> return []

SGTM. Done.

[M-A Ruel, 2011-01-18 16:38:07]

lgtm

http://codereview.chromium.org/6299001/diff/32001/ceee/PRESUBMIT.py
File ceee/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/32001/ceee/PRESUBMIT.py#newcode17
ceee/PRESUBMIT.py:17: return ceee_presubmit.CheckChange(input_api,
nit: this line fits 80 cols so no need to split it on 3.

[Jói, 2011-01-19 00:24:09]

Thanks Marc-Antoine. Sorry about the presubmit syspath thing guys. It was done
this way to be able to reuse the same presubmit logic in multiple locations, see
e.g. chrome_frame/PRESUBMIT.py

Cheers,
Jói


On 2011/01/18 16:38:07, Marc-Antoine Ruel wrote:
> lgtm
> 
> http://codereview.chromium.org/6299001/diff/32001/ceee/PRESUBMIT.py
> File ceee/PRESUBMIT.py (right):
> 
> http://codereview.chromium.org/6299001/diff/32001/ceee/PRESUBMIT.py#newcode17
> ceee/PRESUBMIT.py:17: return ceee_presubmit.CheckChange(input_api,
> nit: this line fits 80 cols so no need to split it on 3.

[Jakob Kummerow, 2011-01-19 10:06:33]

Thanks for the reviews. I'll land this now.

On 2011/01/19 00:24:09, Jói wrote:
> Thanks Marc-Antoine. Sorry about the presubmit syspath thing guys. It was done
> this way to be able to reuse the same presubmit logic in multiple locations,
see
> e.g. chrome_frame/PRESUBMIT.py

Fixed that one, too.

http://codereview.chromium.org/6299001/diff/32001/ceee/PRESUBMIT.py
File ceee/PRESUBMIT.py (right):

http://codereview.chromium.org/6299001/diff/32001/ceee/PRESUBMIT.py#newcode17
ceee/PRESUBMIT.py:17: return ceee_presubmit.CheckChange(input_api,
On 2011/01/18 16:38:14, Marc-Antoine Ruel wrote:
> nit: this line fits 80 cols so no need to split it on 3.

Done.