Summary:

Policy support backend

Review Request #970 — Created Aug. 17, 2009 and discarded

Information
Owner:	edufelipe
Repository:	Review Board SVN (deprecated)
Branch:
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

This patch includes policy support for ReviewBoard. It includes the expression parser, a simple admin interface and a single action as a proof of concept.

This is a fully extensible parser, that already has a few operators to serve as an example and more operators can be used in the future.
The parser is based on a technique described in the book Beautiful Code, and has the advantage of being fast and creating a syntactic tree that can be evaluated under different contexts, and the disadvantage of having poor error messages for syntactic errors. This can be improved in latter versions using heuristics.

Actions should be checked through the helper function policy.applier.applies, conviniently imported under the name `policy_checks` in the example. That function was caching syntactic trees, but I couldn't get the cache invalidated, so I removed it.

I still don't know wether we should be using a reference to a string in the Policy class or a hardcoded string for action names, but in order to simplify the development I opted for the latter. More actions will have to be mapped for this patch to be considered complete, and those will come in future updates to this review.

Also, there should be enough semantic in the operators themselves that an builder is feasible for a limited number of expressions. Also, the manual should be automatically generated, so should a syntax helper on the side of the screen.

Testing Done

The parser was tested a lot. The rest was tested only in Safari.

In general, this change looks awesome, but there's a lot of code style changes I'd like to see made. Also, a lot of doc blocks could use some revising.

/trunk/reviewboard/admin/views.py (Diff revision 1)

Should be in the list alphabetically.

/trunk/reviewboard/admin/views.py (Diff revision 1)

Should continue to be two blank lines here.

/trunk/reviewboard/policy/admin.py (Diff revision 1)
```
Two blank lines.
```

/trunk/reviewboard/policy/admin.py (Diff revision 1)

This can be in the class itself.

/trunk/reviewboard/policy/admin.py (Diff revision 1)
```
No blank line here.
```
/trunk/reviewboard/policy/applier.py (Diff revision 1)
```
Should remove this line.
```
/trunk/reviewboard/policy/applier.py (Diff revision 1)
```
Two blank lines here.
```
/trunk/reviewboard/policy/applier.py (Diff revision 1)
```
Can you add a doc block explaining this?
```
/trunk/reviewboard/policy/applier.py (Diff revision 1)
```
Can combine these lines.
```
/trunk/reviewboard/policy/applier.py (Diff revision 1)
```
Blank line here.
```
/trunk/reviewboard/policy/applier.py (Diff revision 1)
```
Remove these lines.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Should be alphabetical.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Two blank lines here.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

You should be able to make this simply inherit from Exception. It should handle taking a string automatically as part of Exception.

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Two blank lines here.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

I'd prefer a bit of cleanup with the comment. The "Nothing too fancy here" isn't really helpful from a user's point of view.

The comment about the literal strings can just be a standard # comment before the tokenizer.

/trunk/reviewboard/policy/expression.py (Diff revision 1)

I think you can just simply return self.tokenizer.finditer(self.origin)

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

Much of this doc block is about the implementation and background of the class, rather than how it's used. The doc block should be more about the purpose of the class and how to use it. The rest should go in standard comments.

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

Instead of using \ to join multi-line conditionals, put the whole thing in parens.

/trunk/reviewboard/policy/expression.py (Diff revision 1)

No need for the \, since it's already in parens.

Align the % (token, ...) with the string.

/trunk/reviewboard/policy/expression.py (Diff revision 1)

What's this symbol? Should probably be a constant instead.

/trunk/reviewboard/policy/expression.py (Diff revision 1)

"If fails" should probably be something like "If the token is not an operator, then it's a variable and will be returned."

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Same here. Use Variable.name.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line between these.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Shouldn't be a blank line here.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

For readability, I'd prefer: if ' ' not in op_name

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Can you put the comment on the next line?
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Parameters should be aligned.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
"or a variable"
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line should be removed.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
No need for the comma after "constructs"
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
"its" instead of "it's"
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

This sentence doesn't make much sense to me. Also, maybe this is a regional difference, but it should be "it" instead of "he". Same with other comments in this file.

/trunk/reviewboard/policy/expression.py (Diff revision 1)

Would be nice to use _(..) for all strings in this file.

/trunk/reviewboard/policy/expression.py (Diff revision 1)

No space after """. Same with other doc blocks.

Also, "first," not "fist". Same in right().

I'm not really sure what the comment means though. Can you reword it?

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
No r before """
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
"... of Django's own Variable for templates."
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
No blank line here.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

Can you put the comment on its own line? Also, comments should be in sentence casing.

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Same here.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Blank line here.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

Same about the comment. Also the same throughout the rest of this code.

/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Can combine these.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Can you add a doc block?
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Two blank lines.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Two blank lines.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Two blank lines.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
its, not it's
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
What is 70 for? Should probably use a constant.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Two blank lines.
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Needs a doc block
```
/trunk/reviewboard/policy/expression.py (Diff revision 1)
```
Need a doc block.
```

/trunk/reviewboard/policy/expression.py (Diff revision 1)

Should be "expression" or "create_expression" or something.

/trunk/reviewboard/policy/models.py (Diff revision 1)
```
No blank line here.
```
/trunk/reviewboard/policy/models.py (Diff revision 1)
```
Two blank lines here.
```
/trunk/reviewboard/policy/models.py (Diff revision 1)
```
"reviewboard" should be "Review Board"
```

/trunk/reviewboard/policy/models.py (Diff revision 1)

Seems like part of the second sentence here is missing.

/trunk/reviewboard/policy/models.py (Diff revision 1)
```
Should be unindented one indentation level.
```

/trunk/reviewboard/policy/models.py (Diff revision 1)

The "default" parameter should be aligned with the other parameters.

/trunk/reviewboard/policy/models.py (Diff revision 1)
```
Same with the first parameter here.
```

/trunk/reviewboard/policy/models.py (Diff revision 1)

Can you add a TODO here and file a bug to get both the docs written and a link to the docs from this help text?

/trunk/reviewboard/policy/models.py (Diff revision 1)

No need for the \, and the second line of params should align with the error string.

/trunk/reviewboard/policy/models.py (Diff revision 1)
```
Should be localized using _(..)
```
/trunk/reviewboard/policy/models.py (Diff revision 1)
```
Should remove these two lines.
```

/trunk/reviewboard/policy/rules.py (Diff revision 1)

These imports should be absolute. reviewboard.policy.expression. You can run into interesting problems otherwise if importing the same module using two separate relative paths.

/trunk/reviewboard/policy/rules.py (Diff revision 1)
```
Two blank lines here.
```
/trunk/reviewboard/policy/rules.py (Diff revision 1)
```
Two blank lines here.
```

/trunk/reviewboard/policy/rules.py (Diff revision 1)

This would be fine for a # comment, but not a doc block. Can you add a doc block explaining what this class does?

/trunk/reviewboard/policy/rules.py (Diff revision 1)
```
Two blank lines here.
```
/trunk/reviewboard/policy/rules.py (Diff revision 1)
```
Remove these lines.
```

/trunk/reviewboard/policy/tests.py (Diff revision 1)

If we don't have unit tests yet, we shouldn't add this file.

/trunk/reviewboard/reviews/models.py (Diff revision 1)

Not a huge fan of this. It makes it harder to grep for usage of functions and stuff.

Maybe rename applies to policy_applies in applier.py.

/trunk/reviewboard/policy/models.py (Diff revision 1)

Don't you think it's too soon to file a bug about this?

/trunk/reviewboard/policy/rules.py (Diff revision 1)

Yeah, I complained about that on the other review and made that mistake here. Sorry.