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.

Sign up for GitHub

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

Jump to bottom

Run docformatter==v1.6.1 #1886

Merged
merged 2 commits into from
Apr 22, 2023
Merged

Run docformatter==v1.6.1 #1886

merged 2 commits into from
Apr 22, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Apr 22, 2023 •
edited
Loading

Changes

  • Ran docformatter v1.6.1 over the entire code base manually coaxing a few spots to play along
  • Specific docformatter config:
  - repo: https://github.com/PyCQA/docformatter
    rev: v1.6.1
    hooks:
      - id: docformatter
        args: [--in-place, --wrap-descriptions=88, --wrap-summaries=88]

Notes

  • Based on this discussion, I did not re-enable docformatter in general
  • Also, just saw that v1.6.2 was released; it does not produce anything additional changes.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16
Copy link
Member Author

If you don't want to encourage PRs like this just to review a bunch of docstrings, feel free to close this :)

rmartin16
rmartin16 commented Apr 22, 2023
View reviewed changes
examples/positron-django/src/webapp/settings.py
Comment on lines -5 to +9
For more information on this file, see
https://docs.djangoproject.com/en/4.1/topics/settings/
For more information on this file, see:
- https://docs.djangoproject.com/en/4.1/topics/settings/

For the full list of settings and their values, see
https://docs.djangoproject.com/en/4.1/ref/settings/
For the full list of settings and their values, see:
- https://docs.djangoproject.com/en/4.1/ref/settings/
Copy link
Member Author

Choose a reason for hiding this comment

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

docformatter really wanted to insert a bunch of newlines here so i added some symbols to try to stop that from happening.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting that it doesn't add ticks around the URL like it did in the briefcase patch

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh....i actually did that manually in the briefcase update. I had forgotten I did that.

freakboy3742
freakboy3742 approved these changes Apr 22, 2023
View reviewed changes
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I've made one very minor tweak for consistency, but otherwise this all looks good. I guess a periodic manual pass of docformatter will help catch the most common issues, and we can manually fix any ReST issues we find.

core/src/toga/handlers.py Show resolved Hide resolved
core/src/toga/widgets/base.py Outdated
widget?"""
"""Is the widget currently enabled?

i.e., can the user interact with the widget?
Copy link
Member

Choose a reason for hiding this comment

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

Breaking this sentence up doesn't make much sense; not sure why it's been broken here, but not in other identical sentences on other widgets.

Copy link
Member Author

Choose a reason for hiding this comment

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

The first line of a docstring should be one complete sentence. Subsequent sentences will be separated by an empty line....as least AFAIK....

Copy link
Member

Choose a reason for hiding this comment

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

Sure - that's my understanding as well... but it didn't do this on Box (and a couple of others). It's the inconsistency that is weirding me out.

core/src/toga/widgets/box.py
@@ -29,17 +29,17 @@ def __init__(

@property
def enabled(self):
"""Is the widget currently enabled? i.e., can the user interact with the
widget?
"""Is the widget currently enabled? i.e., can the user interact with the widget?
Copy link
Member

Choose a reason for hiding this comment

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

I'm not clear why this didn't get split into 2 lines where the implementation on base did.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm....my guess is because it has an empty line and more text following. So, I guess the "one sentence on first line" rule isn't being enforced here....

examples/positron-django/src/webapp/settings.py
Comment on lines -5 to +9
For more information on this file, see
https://docs.djangoproject.com/en/4.1/topics/settings/
For more information on this file, see:
- https://docs.djangoproject.com/en/4.1/topics/settings/

For the full list of settings and their values, see
https://docs.djangoproject.com/en/4.1/ref/settings/
For the full list of settings and their values, see:
- https://docs.djangoproject.com/en/4.1/ref/settings/
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that it doesn't add ticks around the URL like it did in the briefcase patch

@freakboy3742 freakboy3742 merged commit a210ba4 into beeware:main Apr 22, 2023
@rmartin16 rmartin16 deleted the docformatter branch April 22, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freakboy3742 freakboy3742 freakboy3742 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rmartin16 @freakboy3742