pocket-lint

Merge lp://staging/~adiroiban/pocket-lint/1237862-pep-257 into lp://staging/pocket-lint

  1. 1237862-pep-257
  2. Merge into trunk
Proposed by Adi Roiban
Status: Merged
Merged at revision: 443
Proposed branch: lp://staging/~adiroiban/pocket-lint/1237862-pep-257
Merge into: lp://staging/pocket-lint
Diff against target: 180 lines (+120/-1)
2 files modified
pocketlint/formatcheck.py (+28/-1)
pocketlint/tests/test_python.py (+92/-0)
To merge this branch: bzr merge lp://staging/~adiroiban/pocket-lint/1237862-pep-257
Reviewer Review Type Date Requested Status
Curtis Hovey code Approve
Adi Roiban (community) Needs Resubmitting
Review via email: mp+194239@code.staging.launchpad.net

Description of the change

Why
---

pep257 looks like a nice effort in docstring format checking.

Changes
-------

Add optional pep257 check if module is present.

I added pep257 as setup.py dependency, but maybe we should remove it.

pep257 has no errror codes yet (wip: https://github.com/GreenSteam/pep257/pull/53) so the filtering is done on message string.

I also made a drive-by fix for pdb check.

Please let me know if changes make sense.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Adi.

The tests don't pass when pep257 is not installed. We don't require the lib so I think the tests should skip.

I am not sure about the requires pep257. It is not packaged in Ubuntu. The dh will report an error when it calls setup to install during the package build. While I can copy the pep257 from someone-else's archive, pocket-lint will be removed from Ubuntu unless someone commits to creating python3-pep257 and possibly python2-pep257. I'd think removing pep257 from requires will prevent regressions.

pocket-lint now wrongly reports that a pdb is left in the code. Unless you know how to update the pdb check to know the string is not a function, this block needs to be restored.
- pdb_call = 'pdb.' + 'set_trace'
- if pdb_call in line:

review: Needs Information (code)
Revision history for this message
Adi Roiban (adiroiban) wrote :

Thanks for the review.

pep257 is still young and unstable so I don't think that it needs to be an Ubuntu package.

I will push a new change removing the install requirement and also skipping the tests when pep257 is not there.

I will also revert the pdb_call variable, but I think that it needs a comment. Otherwise is just triggers a WTF moment.

lp://staging/~adiroiban/pocket-lint/1237862-pep-257 updated
512. By Adi Roiban <email address hidden>

Fix after review.

Revision history for this message
Adi Roiban (adiroiban) wrote :

Please check latest changes.

Thanks!

review: Needs Resubmitting
Revision history for this message
Adi Roiban (adiroiban) wrote :

When you have time, please check latest changes. Thanks!

lp://staging/~adiroiban/pocket-lint/1237862-pep-257 updated
513. By Adi Roiban <email address hidden>

Merge branch 'master' into 1237862-pep-257

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you Adi.

I am very sorry for ignoring your reviews. I saw you updated the reviews while I was travelling. I was so exhausted when I returned home, I forgot about them. I should have set a reminder.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: