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

add Google style docstring to web.py #746

Merged
merged 6 commits into from
Nov 22, 2018

Conversation

GrugLife
Copy link
Contributor

@GrugLife GrugLife commented Nov 21, 2018

Description

_Add Google style docstrings to web.py per the following link:
https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

Please let me know of any changes to the current docstrings.

If all looks good, I will tackle another file.
_

#532 (web.py)

Status

READY

Type of change

  • Documentation (fix or adds documentation)

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (if applicable)
  • [NA] I have added tests that prove my fix is effective or that my feature works
  • [NA] New and existing unit tests pass locally with my changes

@jacobtomlinson
Copy link
Member

Thanks for this!

The linter is raising some issues with having blank lines after your docstrings. Could you please address this.

lint runtests: commands[2] | pydocstyle opsdroid tests
opsdroid/web.py:35 in public method `get_port`:
        D202: No blank lines allowed after function docstring (found 1)
opsdroid/web.py:56 in public method `get_host`:
        D202: No blank lines allowed after function docstring (found 1)
opsdroid/web.py:74 in public method `get_ssl_context`:
        D202: No blank lines allowed after function docstring (found 1)
opsdroid/web.py:115 in public method `build_response`:
        D202: No blank lines allowed after function docstring (found 1)
opsdroid/web.py:152 in public method `web_index_handler`:
        D202: No blank lines allowed after function docstring (found 1)

@FabioRosado
Copy link
Member

I am a bit confused by the linting issues, with the google style docstrings you should have the docstrings like:

    def build_url(self, method):
        """Build the url to connect with api.

        Helper function to build the url to interact with the
        Rocket.Chat REST API. Uses the global variable API_PATH
        that points to current api version. (example: /api/v1/)

        Args:
            method (string): Api call endpoint.

        Return:
            String that represents full API url.

        """

Unless pylint got updated and they don't accept this style anymore?

@jacobtomlinson
Copy link
Member

jacobtomlinson commented Nov 21, 2018

But there shouldn't be a blank line between the last """ and the first line of code. Like here

@FabioRosado
Copy link
Member

Oh you are right haha I thought that the lint was complaining about the line between the first docstring line and the Args, bit.

@GrugLife
Copy link
Contributor Author

Sure thing, will get on it.

@codecov
Copy link

codecov bot commented Nov 21, 2018

Codecov Report

Merging #746 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #746   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          28     28           
  Lines        1843   1843           
=====================================
  Hits         1843   1843
Impacted Files Coverage Δ
opsdroid/web.py 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a9059e...c2e0185. Read the comment docs.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are still some linter failures.

opsdroid/web.py Outdated
request: web request to render opsdroid stats

Returns:
dict: returns successful status code and dictionary with stats requested
Copy link
Member

@jacobtomlinson jacobtomlinson Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this line is too long. Could you add a linebreak to make it less that 80 characters.

@GrugLife
Copy link
Contributor Author

agggh, I thought I had it.

Is there a way to check for this before I push? I ran it through pylint and it checked out.

Thanks,
Greg

@FabioRosado
Copy link
Member

FabioRosado commented Nov 21, 2018

@GrugLife No worries, its all a learning experience 😄 👍
Yeah you can check this if you run tox from your command line (you might need to pip install it).

Basically cd into your opsdroid directory, type tox and the tests will run, you will see all the tests that passed/failed, the % of test coverage and which lines are not covered and any linting issues you might have.

If you get a message saying you dont have tox, just install it and run the command again

Copy link
Member

@FabioRosado FabioRosado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya thanks for working on this. You don’t need to add the \ on the dock strings you can just hit enter to separate the line and everything will work fine. So you could just remove the r from the beginning of the docstring

@jacobtomlinson
Copy link
Member

🎉 Thanks for bearing with us! We have quite strict linting rules in this project but I'm glad to see we got there in the end.

Ping me your address via DM on Twitter or Gitter and I'll post you some opsdroid stickers!

@jacobtomlinson jacobtomlinson merged commit 47ef8bc into opsdroid:master Nov 22, 2018
@GrugLife
Copy link
Contributor Author

No worries, it's a good learning experience to work within the linting rules. And thanks for bearing with me!

If you don't mind, I would like to do the same on another file.

@jacobtomlinson
Copy link
Member

Please go ahead!

FabioRosado pushed a commit to FabioRosado/opsdroid that referenced this pull request Dec 18, 2018
* add Google style docstring to web.py

* delete trailing blank line after docstring

* Fix lintr failures

* add r for docstring with \

* removed r and \
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants