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

Cleanup of documentation #2898

Merged
merged 5 commits into from Jun 1, 2021
Merged

Cleanup of documentation #2898

merged 5 commits into from Jun 1, 2021

Conversation

krisgesling
Copy link
Contributor

@krisgesling krisgesling commented May 5, 2021

Description

Improve documentation including standardizing docstrings and address warnings raised by the Sphinx readthedocs builder, as well as fixing up issues in CLI client.

  • We currently go by the Google Python Docstrings Style Guide which uses Args: rather than Arguments:. This has been inconsistently used throughout the code base so this modifies them all to be the standard.
  • Fixed heading underlines to silence warnings
  • Added missing lines to silence warnings and aid readability
  • Improved formatting of particular items eg mycroft.MycroftSkill.get_response had missing content, and now provides highlighted examples.
  • Fixed capitalization of "Skills" in the CLI help info
  • Fixed incorrect colors on Log Output Legend
  • Fixed incorrect keyboard shortcuts for next and previous utterances in CLI
  • ran autopep8 on touched files to address auto-format.

How to test

source .venv/bin/activate
cd doc
make html

Also check the CLI client

Contributor license agreement signed?

@pep8speaks
Copy link

pep8speaks commented May 5, 2021

Hello @krisgesling! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-11 05:42:51 UTC

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 5, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator

forslund commented May 5, 2021

Nice work. Looks very clean and get_response() looks great!

I do however still get a bunch of warnings:
/home/ake/projects/python/mycroft-core/mycroft/util/parse.py:docstring of mycroft.util.parse.normalize:10: WARNING: Block quote ends without a blank line; unexpected unindent.

Are these intended to be left in?

IMO the auto-pep8 should be run in a separate commit but not a big deal here.

EDIT:
The warnings seems to be fixable by adding blank lines between the different section (after first line summary, after body and after argument list) See here

EDIT2: The commonIOT skill also has an undefined type-hint, I don't know very much of type-hints so I'm not sure what's the best way to handle that.

EDIT3: it wasn't the type-hint that was the issue, rather the _ after request triggering some weird internal warning. See here

@krisgesling
Copy link
Contributor Author

Indeed - need to PEP8 the whole repo so it doesn't catch you when you're updating something completely separate...
I might at least remove text_client.py and do that separately given the auto-format changes are quite extensive.

The remaining warnings for parse and format docstrings should no longer exist in this repo with the inclusion of LF v0.4.1.

Nice find on the underscore issue!

@forslund
Copy link
Collaborator

forslund commented May 5, 2021

Then they were left intentionally. Feel free to add the common_iot_skill fix. And merge away when the PEP-8 line length issues have been fixed :)

forslund
forslund previously approved these changes May 5, 2021
@krisgesling krisgesling changed the title Docstring and docs rst cleanup Cleanup of documentation May 7, 2021
@krisgesling
Copy link
Contributor Author

hmm soz... I probably should have done the PEP8 in a separate commit while I was at it, but I'm a terrible person 😈

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@krisgesling
Copy link
Contributor Author

Timer Skill you are drunk! :P

@forslund
Copy link
Collaborator

forslund commented May 7, 2021

Timer skill is definitely the black sheep among the skills. Always the drunk one and always the loudest :P

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

Copy link
Member

@chrisveilleux chrisveilleux left a comment

Choose a reason for hiding this comment

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

Hard to argue with this change... oh I crack myself up. Nice work Gez.

@krisgesling krisgesling merged commit 1cc25da into dev Jun 1, 2021
@krisgesling krisgesling deleted the bugfix/docstring-cleanup branch June 1, 2021 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants