-
Notifications
You must be signed in to change notification settings - Fork 279
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 docs for FieldValidator objects #4509
improve docs for FieldValidator objects #4509
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. Here are a couple comments
yt/fields/derived_field.py
Outdated
If *parameter_values* is supplied, this will also ensure that the field | ||
Parameters | ||
---------- | ||
parameters: Union[str, Iterable[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually don't use type annotation syntax in docstrings. In particular Union
and Optional
should be avoided there because they are not needed since Python 3.10 (these annotations will be simplified with ruff (pyupgrade) when Python 3.9 is dropped).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed all the Union and Optional occurrences. left the iterable though? Should I remove that too? it is nice to note that it could be a str or any iterable with str's. but I could remove the Iterable
and rephrase the description
yt/fields/derived_field.py
Outdated
@@ -589,3 +625,14 @@ def __call__(self, data): | |||
if getattr(data, "_type_name", None) == "grid": | |||
return True | |||
raise NeedsOriginalGrid() | |||
|
|||
|
|||
def _set_field_validator_base_docstring(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be suited for FieldValidator.__init_subclass__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea! i'll move it and refactor a bit as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now in __init_subclass__
Co-authored-by: Clément Robert <cr52@protonmail.com>
Co-authored-by: Clément Robert <cr52@protonmail.com>
This PR:
FieldValidator
subclasses up out of their__init__
methods so they get picked up by the docs buildFieldValidator
docstring to point to the available subclasses.Background: I was working using one of the
FieldValidator
and noticed that all the docstrings here were empty despite their presence in the__init__
methods. Thought it'd be useful to bump those up so we catch them during the docs build... and then I made an effort to improve the docstrings.I'm not super familiar with the different validators, so it's possible I misinterpreted the arguments (both the descriptions and any typing I added).
Also, this includes one small code refactoring as well as a little function to dynamically update the
FieldValidator
docstring -- I'm happy to remove those if it's better to simplify this PR.