ENH: Add more text to registry.search docstring for the types of constraints that can be used

[zoghbi-a]

This is an enhancement to the registry search docstring. It adds a list of constraints that can be used.

The purpose of these additions is to make the docstring sufficient for the user who is not familiar with the available constraints to use the search method without a need to dig the documentation to find them.

I copied the relevant text from the the documentation page

[bsipocz]

Thanks. One minor comment, and also please remove training writespaces

Unrelated to the PR: I wonder whether anyone would be against adding all these as optional arguments rather than hide them in *constraints?

(from the API point of view I'm very much against the practice of *args, **kwargs, as more often than not they hide away bugs and usage errors, and if there is a finite set of possibilities that can be passed in those iterables)

[bsipocz]

Add an indent here and for all the other items in the list to make the rendering nicer.

Suggested change

	- keywords: one or more freetext words, mached in the title,
	- keywords: one or more freetext words, mached in the title,

[zoghbi-a]

Is the updated version better?

[bsipocz]

yes. The whitepace linting issue is still there, as well as some extra whitespace with the rendering (https://pyvo--426.org.readthedocs.build/en/426/api/pyvo.registry.regtap.search.html#pyvo.registry.regtap.search), I'll just add a commit that fixes all those rather than going back and forth with it.

[bsipocz]

here it is with the new commit:
https://pyvo--426.org.readthedocs.build/en/426/api/pyvo.registry.regtap.search.html#pyvo.registry.regtap.search

Apparently, the rendering reuses the link and only serves the latest version.

[msdemlei]

On Tue, Feb 21, 2023 at 03:04:06PM -0800, Abdu Zoghbi wrote: This is an enhancement to the registry search docstring. It adds a list of constraints that can be used. The purpose of these additions is to make the docstring sufficient for the user who is not familiar with the available constraints to use the search method without a need to dig the documentation to find them.

Thanks, I'm sure many people will appreciate that. What worries me a bit (and that's the reason I initially tried to get away with a link to the list of constraints) is how to keep this up to date as constraints are added or changed. If anyone reading this has a good (and preferably pragmatic) idea, please let me know.

[bsipocz]

What worries me a bit (and that's the reason I initially tried to get
away with a link to the list of constraints) is how to keep this up
to date as constraints are added or changed. If anyone reading this
has a good (and preferably pragmatic) idea, please let me know.

I would make them mandatory keyword arguments. That would allow us to easily insert an additional one, and would require people to be explicit. As to removing, or renaming then we can use the astropy deprecation procedure for renaming/removing parameters.

[codecov]

Codecov Report

Merging #426 (2bb3d54) into main (f10aff5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #426   +/-   ##
=======================================
  Coverage   79.81%   79.81%           
=======================================
  Files          52       52           
  Lines        5989     5989           
=======================================
  Hits         4780     4780           
  Misses       1209     1209           

Impacted Files	Coverage Δ
pyvo/registry/regtap.py	79.03% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[msdemlei]

On Wed, Feb 22, 2023 at 09:20:35AM -0800, Brigitta Sipőcz wrote: I would make them mandatory keyword arguments. That would allow us

Hm... the way this works here is that positional arguments are treated differently from keyword arguments; positional arguments are expected to be Constraint instances (and that's where I'd like to nudge people towards, but that may be a fool's errand), keyword arguments are, if you will, syntactic sugar over these. But no, my concerns were how to keep the documentation in the two (actually three) places that list the Constraints in sync. I'll sneak in an amendment to the comment in line 575 of rtcons.py in the PR for sia2 -- perhaps that's fine.

[bsipocz]

I go ahead with the merge as the discussion side-tracked towards how to better expose the constraints to the API. I still think that's the way forward, but in the meantime documenting the currently possible constraints is an improvement.

[bsipocz]

Thanks @zoghbi-a!