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

Improve info on pulp-glue and adding version-constrain to option (on Contributing) #836

Closed
pedro-psb opened this issue Nov 28, 2023 · 0 comments · Fixed by #837
Closed
Assignees
Labels
Documentation Improvements or additions to documentation Triage-Needed Needs to be reviewed at next pulp-cli mtg

Comments

@pedro-psb
Copy link
Member

pedro-psb commented Nov 28, 2023

Summary

When working on the drop of #831 Matthias clarified some history and practical info which would be useful in the Contributing section:

  • Introduction to pulp-glue and why it's important
  • How-to add option version-constrains (on pulp-glue)
  • Recommend pulp_option more generally for declaring options (over click.option)

Examples

pulp_option provides a superset of features over click.option. Since this project, as many others, grew organically, it is not quite used consistently in all places. To use things like needs_plugin, a pulp_option is absolutely necessary. I'm still on the fence whether we should recommend using it in (almost; --config is an exception for some reason as far as i remember) all places.

OK, sorry, needs_plugin does not work with click.option, but it still works when the layer below is raising the right exception (Hooray to stack unwinding exception handling!).

Declaring the need at the option level was what we were doing before realizing that pulp_glue had it's independent value as a standalone library. For the sake of the cli it almost does not matter where that exception is raised. But for uses of glue outside of the cli, it does.

I currently believe we should try as best as we can to move (or in your case, add) all version dependency in the glue library so all associated projects can benefit. This also makes the glue library the comprehensive guide to all known Pulp API subtleties.

The question of pulp_option over click.option is therefor in many cases one of best practice and least surprise. I think i'll vote for "use pulp_option" because this make pulpcore.cli.common.generic the library to the pulp-cli plugins and we have some control over subcommands that we loose when they use click.option directly.

@pedro-psb pedro-psb added Documentation Improvements or additions to documentation Triage-Needed Needs to be reviewed at next pulp-cli mtg labels Nov 28, 2023
pedro-psb added a commit to pedro-psb/pulp-cli that referenced this issue Nov 29, 2023
Done in the 'Architecture' section of Contribution docs:
- Add 'Pulp Glue' section
- Add clarificatons about version-guard option/body-params
- Update docstrings of objects that were referenced.
- Nest 'Context' and 'Version Dependent' section under 'Pulp Glue'
- Fix typos

closes pulp#836
pedro-psb added a commit to pedro-psb/pulp-cli that referenced this issue Nov 29, 2023
Done in the 'Architecture' section of Contribution docs:
- Add 'Pulp Glue' section
- Add clarificatons about version-guard option/body-params
- Update docstrings of objects that were referenced.
- Nest 'Context' and 'Version Dependent' section under 'Pulp Glue'
- Fix typos

closes pulp#836
pedro-psb added a commit to pedro-psb/pulp-cli that referenced this issue Nov 29, 2023
Done in the 'Architecture' section of Contribution docs:
- Add 'Pulp Glue' section
- Add clarificatons about version-guard option/body-params
- Update docstrings of objects that were referenced.
- Nest 'Context' and 'Version Dependent' section under 'Pulp Glue'
- Fix typos

closes pulp#836
@pedro-psb pedro-psb self-assigned this Nov 29, 2023
pedro-psb added a commit to pedro-psb/pulp-cli that referenced this issue Dec 12, 2023
Done in the 'Architecture' section of Contribution docs:
- Add 'Pulp Glue' section and next sections 'Context' and
  'Version Dependent' under it.
- Add clarificatons about version-guard to options and body-params.
- Improve info about PulpEntityContext usage.
- Improve `pulp_option` docstrings w/ description and examples.
- Fix typos, re-phrasings and add some internal links.

Co-authored-by: Matthias Dellweg <2500@gmx.de>
closes pulp#836
pedro-psb added a commit to pedro-psb/pulp-cli that referenced this issue Dec 12, 2023
Done in the 'Architecture' section of Contribution docs:
- Add 'Pulp Glue' section and next sections 'Context' and
  'Version Dependent' under it.
- Add clarificatons about version-guard to options and body-params.
- Improve info about PulpEntityContext usage.
- Improve `pulp_option` docstrings w/ description and examples.
- Fix typos, re-phrasings and add some internal links.

Co-authored-by: Matthias Dellweg <2500@gmx.de>
closes pulp#836
mdellweg pushed a commit that referenced this issue Dec 12, 2023
Done in the 'Architecture' section of Contribution docs:
- Add 'Pulp Glue' section and next sections 'Context' and
  'Version Dependent' under it.
- Add clarificatons about version-guard to options and body-params.
- Improve info about PulpEntityContext usage.
- Improve `pulp_option` docstrings w/ description and examples.
- Fix typos, re-phrasings and add some internal links.

Co-authored-by: Matthias Dellweg <2500@gmx.de>
closes #836
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements or additions to documentation Triage-Needed Needs to be reviewed at next pulp-cli mtg
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant