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 docs for the filter reveal_ansible_type and the test ansible_type. #8645

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

vbotka
Copy link
Contributor

@vbotka vbotka commented Jul 16, 2024

SUMMARY

Add below files to docs/docsite/rst:

  • filter_guide-abstract_informations-types.rst
  • filter_guide-abstract_informations-types-reveal_ansible_type.rst

Update the below files in docs/docsite/rst:

  • filter_guide_abstract_informations.rst
  • test_guide.rst
ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@ansibullbot
Copy link
Collaborator

@vbotka
Copy link
Contributor Author

vbotka commented Jul 16, 2024

@russoz, quoting your comment from #8595:

well, personally I still think the test code is more verbose than it needs to be, but I am happy to have it as-is, and we may review it later if needed.

How would you like to change the test code?

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi @vbotka thanks for your contribution! It does look like you imagined a dynamic way to generate documentations within the repository, and that is not something we would like to do. There are processes that read (and expect) .rst files only within that directory.
Moreover, it looks like there are parts of a test for your plugins and tests based in Ansible YAML, as opposed to unit testing python code, should be placed in the tests/integration/targets// directory.
This PR is quite irregular and not in conformance with our practices.
Please review the documentation (a good starting point being: https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md) and see how other documentation was written.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This removes existing documentation for existing plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I restored the origin and added new section.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is out of place. There are scripts and processes that work with the documentation files that will not execute playbooks in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests "roles" should be place in the tests/integration/target directory, not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jul 16, 2024
@russoz
Copy link
Collaborator

russoz commented Jul 16, 2024

@russoz, quoting your comment from #8595:

well, personally I still think the test code is more verbose than it needs to be, but I am happy to have it as-is, and we may review it later if needed.

How would you like to change the test code?

Well, that was then. I have not spent more time on the PR after that commenting. TBH, I will not have much time in the upcoming days. And that question seems to be a bit out of context in this PR.
You are the maintainer of that code, if I make any suggestions in the future you will receive notifications for them.

@vbotka
Copy link
Contributor Author

vbotka commented Jul 16, 2024

This PR is quite irregular and not in conformance with our practices.

Hi @russoz. Thank you for the review!

There are only docs/docsite/rst/*.rst files and BOTMETA.yml changes left. This should be in conformance now.

.github/BOTMETA.yml
docs/docsite/rst/filter_guide-abstract_informations-types-reveal_ansible_type.rst
docs/docsite/rst/filter_guide-abstract_informations-types.rst
docs/docsite/rst/filter_guide_abstract_informations.rst
docs/docsite/rst/test_guide.rst

@vbotka vbotka requested a review from russoz July 16, 2024 12:43
@vbotka vbotka mentioned this pull request Jul 16, 2024
@vbotka
Copy link
Contributor Author

vbotka commented Jul 16, 2024

@russoz, would it be possible to review also ##8647? This removes other helpers I created in the past. Sorry for the noise.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch labels Jul 16, 2024
@felixfontein
Copy link
Collaborator

Hi @vbotka thanks for your contribution! It does look like you imagined a dynamic way to generate documentations within the repository, and that is not something we would like to do.

Why not? I think it's a good idea to generate example outputs in documentation using the actual code, if possible.

(It would be great if there would be a more generic way to do this instead of having to do so much templating, but right now there isn't. There used to be https://github.com/ansible-community/sphinx-ansible, but that's dead, and wouldn't have worked here anyway since the RST files are consumed by Sphinx without that extension.)

Moreover, it looks like there are parts of a test for your plugins and tests based in Ansible YAML, as opposed to unit testing python code, should be placed in the tests/integration/targets// directory.

I agree that testing should not be mixed up with documentation generation. (I already mentioned that in this thread: #8482 (comment))

@@ -1460,6 +1460,10 @@ files:
maintainers: vbotka
docs/docsite/rst/filter_guide_abstract_informations_merging_lists_of_dictionaries.rst:
maintainers: vbotka
docs/docsite/rst/filter_guide-abstract_informations-types.rst:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use underscores:

Suggested change
docs/docsite/rst/filter_guide-abstract_informations-types.rst:
docs/docsite/rst/filter_guide-abstract_informations_types.rst:

Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this needed?

The name of the file is a concatenation of three headings' levels:

  • filter_guide
  • abstract_informations
  • type

The concatenation by dash "-" keeps this information. I've already created a couple of files this way. Should all of them be renamed?

You kept the first dash. Do you think this one should be replaced too?

For the record: This name is used in filter_guide_abstract_informations.rst

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't catch the dashes in the previous PR. Generally all filenames in docs/docsite/rst/ use underscores instead of dashes. It would be great to keep it that way.

Types
^^^^^

Filters to manage Ansible types
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Filters to manage Ansible types
Filters to manage Ansible types.


.. versionadded:: 9.2.0

**Substitution converts str to AnsibleUnicode**
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a heading, so it should be a heading and not a paragraph with manual formatting applied:

Suggested change
**Substitution converts str to AnsibleUnicode**
Substitution converts str to AnsibleUnicode
-------------------------------------------

Similar below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would violate the heading levels:

community.general Filter Guide
==============================

Abstract transformations
------------------------

Types
^^^^^

Filter reveal_ansible_type
""""""""""""""""""""""""""

I'm afraid there are no more levels left.

Copy link
Collaborator

Choose a reason for hiding this comment

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

RST allows quite a few different levels. You can use ~~~~~~~~ or ..... or ****** or +++++++ or ... then :)


**Substitution converts str to AnsibleUnicode**

* String. AnsibleUnicode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this mean? (I know what it means, but regular users do not.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll improve all the comments.

@ansibullbot ansibullbot added stale_ci CI is older than 7 days, rerun before merging stale_review labels Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch check-before-release PR will be looked at again shortly before release and merged if possible. docs needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR stale_ci CI is older than 7 days, rerun before merging stale_review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants