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 param ordering note to plugins page. #101

Merged
merged 10 commits into from
Mar 13, 2023
Merged

Add param ordering note to plugins page. #101

merged 10 commits into from
Mar 13, 2023

Conversation

agowa
Copy link
Contributor

@agowa agowa commented Feb 24, 2023

Add a note to every filter, test, and lookup to list positional parameters before keyword ones.

Relates to reviewer note ansible/ansible#80078 (comment) within ansible/ansible#80078

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Please add a changelog fragment. Thanks.

src/antsibull_docs/data/docsite/plugin.rst.j2 Show resolved Hide resolved
src/antsibull_docs/data/docsite/plugin.rst.j2 Show resolved Hide resolved
src/antsibull_docs/data/docsite/plugin.rst.j2 Outdated Show resolved Hide resolved
@agowa
Copy link
Contributor Author

agowa commented Feb 25, 2023

I've included your suggestions and also added a changelog.
Btw, the link you provided maybe also needs an update. It says that documentation changes (which I'd consider this to be) are trivial and that trivial doesn't require a changelog fragment ;-)

@felixfontein
Copy link
Collaborator

I've included your suggestions and also added a changelog. Btw, the link you provided maybe also needs an update. It says that documentation changes (which I'd consider this to be) are trivial and that trivial doesn't require a changelog fragment ;-)

You are not changing documentation, but templates (and these are code).

src/antsibull_docs/data/docsite/plugin.rst.j2 Outdated Show resolved Hide resolved
src/antsibull_docs/data/docsite/plugin.rst.j2 Outdated Show resolved Hide resolved
src/antsibull_docs/data/docsite/plugin.rst.j2 Outdated Show resolved Hide resolved
src/antsibull_docs/data/docsite/plugin.rst.j2 Outdated Show resolved Hide resolved
changelogs/fragments/101-plugin-param-ordering-note.yml Outdated Show resolved Hide resolved
changelogs/fragments/101-plugin-param-ordering-note.yml Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

Do you want to make some more updates? I can also push some updates if you don't have time.

There will be a new release of antsibull-docs in a couple of days, it would be nice if this PR can also make it in there.

@felixfontein
Copy link
Collaborator

Changes done by this PR:

  1. Remove period at end of introductionary sentence for 'Positional parameters' and 'Keyword parameters':
    Compare https://ansible.fontein.de/collections/ansible/builtin/combine_filter.html#positional-parameters to https://docs.ansible.com/ansible/devel/collections/ansible/builtin/combine_filter.html#positional-parameters

  2. Add a note for lookups, filters, and tests which both have terms (lookups) resp. positional parameters (filters, tests) together with keyword parameters (all of them)
    See https://ansible.fontein.de/collections/ansible/builtin/combine_filter.html#notes

  3. Rename 'Parameters' for lookups to 'Keyword parameters' and add example what these are.
    Compare https://ansible.fontein.de/collections/community/sops/sops_lookup.html#keyword-parameters to https://docs.ansible.com/ansible/devel/collections/community/sops/sops_lookup.html#parameters

Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

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

I don't have the opportunity at the moment to test this locally but based on the before/after links.. LGTM

@felixfontein felixfontein merged commit 356dc95 into ansible-community:main Mar 13, 2023
@felixfontein
Copy link
Collaborator

@agowa338 thanks for improving the docs!
@samccann thanks for reviewing this!

@agowa agowa deleted the patch-1 branch March 20, 2023 00:53
@felixfontein
Copy link
Collaborator

This is now live on the Ansible devel docsite btw: https://docs.ansible.com/ansible/devel/collections/ansible/builtin/combine_filter.html#notes

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.

3 participants