Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Pylint plugin to check Sphinx docstrings. #948

Merged
merged 1 commit into from
Jun 25, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Jun 24, 2015

Also

  • Updated docstrings which don't pass / are invalid.
  • Integrated plugin with tox lint rule and run_pylint.
  • Fixed a elif _GAECreds is not None and isinstance(credentials, _GAECreds): in gcloud.credentials._get_service_account_name.

For now, pulling directly from the mercurial source repository to get the check_docs pylint extension. The maintainer mentioned this will be released July 15-17, 2015.

See:
http://www.bitbucket.org/logilab/pylint/pull-request/143/

Also
- Updated docstrings which don't pass / are invalid.
- Integrated plugin with `tox lint` rule and `run_pylint`.

For now, pulling directly from the mercurial source
repository to get the `check_docs` pylint extension. The
maintainter mentioned this will be released July 15-17, 2015.

See:
http://www.bitbucket.org/logilab/pylint/pull-request/143/
@review-ninja
Copy link

ReviewNinja

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 24, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Jun 24, 2015

@tseaver Sorry about reviewninja. I was toying around with it and didn't realize it had privileges to comment on GitHub. I have now disabled that.

@@ -53,7 +53,7 @@ commands =
python run_pylint.py
deps =
pep8
pylint
-ehg+https://bitbucket.org/logilab/pylint#egg=pylint

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented Jun 24, 2015

This is kind of an ugly, wide-ranging change to be merging in the middle of other development (it is going to cause conflicts for pretty much any WIP / open PR.

@dhermes
Copy link
Contributor Author

dhermes commented Jun 25, 2015

@tseaver I'm with you on most of that (not sure I buy that it's ugly, but some of the diffs are hard to grok).

But do we ever foresee a time where we won't be in the middle of development? This likely won't cause merge issues because it touches docstrings whereas most code changes will touch code.

Did you have your dataset -> client rename in mind or something else?

As for open PRs, it is definitely going to be an issue, but again, one we can't really avoid. As you can see from these changes, we had a lot of docstrings that weren't actually documenting the methods / classes / functions correctly.

@dhermes
Copy link
Contributor Author

dhermes commented Jun 25, 2015

@tseaver How do you see a merge going? I assume you finished your review?

@tseaver
Copy link
Contributor

tseaver commented Jun 25, 2015

If you can stand the merge problems this issue creates, go ahead and merge (I would be tempted to defer this until a "janitorial" phase, myself).

dhermes added a commit that referenced this pull request Jun 25, 2015
Adding Pylint plugin to check Sphinx docstrings.
@dhermes dhermes merged commit 7dece69 into googleapis:master Jun 25, 2015
@dhermes dhermes deleted the add-pylint-docstring-checker branch June 25, 2015 18:05
@dhermes dhermes mentioned this pull request Jul 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants