Python bindings: Some comment-only tidyups and PEP-8 fixes.
Needs ReviewPublic

Authored by shaheed on Feb 8 2017, 6:06 PM.

Details

Reviewers
skelly
Group Reviewers
Frameworks
Build System
Test Plan

No code changes.

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Lint Skipped
Unit
Unit Tests Skipped
shaheed updated this revision to Diff 11078.Feb 8 2017, 6:06 PM
shaheed retitled this revision from to Some comment-only tidyups and PEP-8 fixes..
shaheed updated this object.
shaheed edited the test plan for this revision. (Show Details)
shaheed set the repository for this revision to R240 Extra CMake Modules.
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptFeb 8 2017, 6:06 PM
Restricted Application added subscribers: Build System, Frameworks. · View Herald Transcript
shaheed retitled this revision from Some comment-only tidyups and PEP-8 fixes. to Python bindings: Some comment-only tidyups and PEP-8 fixes..Feb 8 2017, 6:09 PM
skelly edited edge metadata.Feb 12 2017, 7:35 PM

Is anything here actually a PEP 8 fix?

skelly added inline comments.Feb 12 2017, 7:37 PM
find-modules/rules_engine.py
58

I don't think this should be here.

494

Does the docs match the code if you add this here? Why is this in this commit?

704

I don't think the comments are valuable.

The PEP-8 changes are some blank line changes.

The PEP-8 changes are some blank line changes.

There is exactly one blank line insertion, and it makes this TypedefRuleDb documentation inconsistent with, say, the VariableRuleDb, which doesn't have blank lines in the same spot. Should it? If yes, then please just create a commit which adds the blank lines where the blank lines should be and does not do anything else. That would be a commit which is easily and quickly reviewable. I've said this a few times but it seems to be something you are still not doing.

shaheed added inline comments.Feb 12 2017, 8:49 PM
find-modules/rules_engine.py
58

I disagree. Having this in the code suppresses the false positives which are caused by the "_" used by gettext.

704

I think they are perfectly reasonable.

I think the scope of the changes in this commit are perfectly reasonable. I don't think it is unreasonable to group them either.

shaheed added inline comments.Feb 12 2017, 8:54 PM
find-modules/rules_engine.py
494

Yes. Why not?