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

Updated some incomplete docstrings, added missing docstrings #81861

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from
Open

Updated some incomplete docstrings, added missing docstrings #81861

wants to merge 5 commits into from

Conversation

freewaydev
Copy link

@freewaydev freewaydev commented Oct 3, 2023 •
edited
Loading

SUMMARY

Updated some incomplete docstrings, added missing docstrings

ISSUE TYPE
  • Docs Pull Request
ADDITIONAL INFORMATION

@ansibot ansibot added needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 3, 2023
@ansibot
Copy link
Contributor

ansibot commented Oct 3, 2023

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/playbook/role/definition.py:80:80: W291: trailing whitespace

click here for bot help

bcoca
bcoca reviewed Oct 3, 2023
View reviewed changes
lib/ansible/executor/playbook_executor.py Outdated
@@ -31,6 +31,7 @@
from ansible.template import Templar
from ansible.utils.helpers import pct_to_int
from ansible.utils.collection_loader import AnsibleCollectionConfig

Copy link
Member

Choose a reason for hiding this comment

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

?

bcoca
bcoca requested changes Oct 3, 2023
View reviewed changes
Copy link
Member

@bcoca bcoca left a comment

Choose a reason for hiding this comment

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

I think it is good to add most of these, but we should keep using the formatting we use elsewhere instead of changing function documentation standards. https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/common/text/converters.py#L33

@freewaydev
Copy link
Author

I think it is good to add most of these, but we should keep using the formatting we use elsewhere instead of changing function documentation standards. https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/common/text/converters.py#L33

I appreciate the feedback. I've updated one file as a test to verify the format is correct before fixing the rest of the files in this PR. Can you take a look at lib/ansible/playbook/attribute.py and let me know if this file looks good? thanks!

@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Oct 4, 2023
@bcoca
Copy link
Member

bcoca commented Oct 4, 2023

sample looks good, feel free to continue

@freewaydev
Copy link
Author

sample looks good, feel free to continue

Thanks, I've updated the rest of the PR per the repo formatting standard and verified code formatting using black.

@freewaydev freewaydev requested a review from bcoca October 5, 2023 15:06
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Oct 10, 2023
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 11, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Oct 25, 2023
@Akasurde
Copy link
Member

@freewaydev Could you please rebase this branch and let us know? Thanks,

@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoca bcoca Awaiting requested review from bcoca

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_pr This PR has not been pushed to for more than one year. stale_review Updates were made after the last review and the last review is more than 7 days old.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@freewaydev @ansibot @bcoca @Akasurde @s-hertel