Closed GuillemBarroso closed 2 years ago
Merging #169 (2fab5b1) into master (32bd9bd) will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## master #169 +/- ##
=======================================
Coverage 83.78% 83.78%
=======================================
Files 25 25
Lines 1351 1351
=======================================
Hits 1132 1132
Misses 219 219
Hi @PProfizi,
I've added an actual example of a method with typehinting: plot_contour
method in ResultData
class. It is finally showing the types without having to specify them in the method's description.
Changes description:
Now the documentation generated (check artifacts) is showing the types in the description even though they are not explicitly written.
When you agree and approve this I will try to do the same in dpf-core.
Hi @GuillemBarroso , I took a look at the doc for sphinx auto-typehint and maybe we want to use typehints_defaults='braces'
(since I think we want to use the numpydoc styling) and simplify_optional_unions=False
?
To be coherent with the rest of the doc where optional parameters are marked explicitly as such right next to their type.
There is no mention of those in the pyansys dev guidelines but IMO that might look better than having to write "Optional" with the default value in the docstring, which kind of prevents us from not having to put any type-hinting manually, and thus kind of misses the point of using this extension.
After playing with the extension sphinx-autodoc-typehints
(see doc) and with the help of @PProfizi and @greschd, we have the following options.
The method used as example contains:
test_param
: required parameter with one valid type.test_param2
: required parameter with several valid types.display_option
: optional parameter with only one valid type.option_id
: optional parameter with multiple valid types. **kwargs
The source code looks like this:
Note that Union
and Optional
from typing
package are used as proposed in by sphinx-autodoc-typehints
devs.
sphinx-autodoc-typehints
configuration (typehints_defaults
= None
and simplify_optional_unions
= True
) we get:sphinx-autodoc-typehints
configuration (typehints_defaults
= comma
and simplify_optional_unions
= True
) we get:sphinx-autodoc-typehints
configuration (typehints_defaults
= braces
and simplify_optional_unions
= False
) we get:sphinx-autodoc-typehints
configuration (typehints_defaults
= braces-after
and simplify_optional_unions
= False
) we get:Unfortunately, sphinx-autodoc-typehints
does not translate "Union" to "|" or "," so we won't be able to render it to something like this: option_id (int
, float
, optional
). Or even to translate None
with Optional
when simplify_optional_unions
= True
in approach 1. Note that, as they say in their doc, when Optional only has one valid type, it will always display Optional and not (int
, None
) as in the display_option
parameter in approach 1.
Regarding the typehints_defaults
option, we already have the information in the signature... so not sure which option I like the most.
Now the idea is to decide if we want to use sphinx-autodoc-typehints
and if so, which parameter combination. @MaxJPRey, what do you think? Feel free to tag more people for further input. As soon as we decide what to do I'll finish this PR and propose a PR for the dev-guide explaining the chosen criterion.
@RobPasMue You could be interested in that for your geometry project.
Adding @akaszynski because it could be used for other projects.
I don't have a super strong opinion on the options, but would probably go for simplify_optional_unions = True
and typehints_defaults = None
because it ends up less cluttered.
For the typehints_defaults
, I would either go with comma
or braces-after
.
@GuillemBarroso if possible, can you add some screenshots of that to be able to compare with the same piece of code?
For the
typehints_defaults
, I would either go withcomma
orbraces-after
.@GuillemBarroso if possible, can you add some screenshots of that to be able to compare with the same piece of code?
Done. Please, see comment above.
I agree with @MaxJPRey - for me it is more clear to see the typehints_defaults
in the docstring. And personally I prefer the braces-after
option =)
for me it is more clear to see the
typehints_defaults
in the docstring.
Agreed
And personally I prefer the
braces-after
option =)
Personally I tend to prefer comma
as the information is right next to what it is about.
PS: the point of having typehints_defaults
different from None is to be coherent with what was done before as the default value was explicitly put in the dosctring. At least with this we can automate this, so I wouldn't remove it altogether.
As for the Union, I do not have an opinion, but I guess simplify_optional_unions = True
would be ok.
As for the Union, I do not have an opinion, but I guess
simplify_optional_unions = True
would be ok.
I just find it a bit awkward to see both "Optional" and "None" words for optional parameters depending if we have one or several valid types.
Personally, I would go with the 4th approach.
As for the Union, I do not have an opinion, but I guess
simplify_optional_unions = True
would be ok.I just find it a bit awkward to see both "Optional" and "None" words for optional parameters depending if we have one or several valid types.
Let's use simplify_optional_unions = False
then and we will change later if the guideline tends to go another way.
Hi, just joining this interesting discussion.
My only concern is about not following the numpydoc style, as I see that the type hints in the Parameters
section had to be removed.
We should investigate an approach to just modify our conf.py
while style being compliant with numpydoc.
This issue is related with https://github.com/numpy/numpydoc/issues/196
not following the numpydoc style
FMPOV, the advantage of not having to repeat the types in the docstring (and them potentially being stale!) outweighs being non-compliant with the numpydoc style. As for the style checker, is there a way we can ignore this specifically?
As mentioned in the discussion today: the type hints should only be the preferred way if they are checked (e.g. by mypy). Otherwise, having the types in the docstring only is still the preferred approach.
Changes description:
.gitignore
: back to ignoring docs/build and not _build as appears in the Makefile.conf.py
: for now I leave the parameters as: simplify_optional_unions = False
and typehints_defaults="comma"
.result_data.py
: using type hints in `plot_contour
method.I will prepare an example using typehinting in conjuntion with mypy in the dev-guide, see pyansys/dev-guide#187.
I think this is now ready to merge.
Let's make a poll here: Upvote your preferred option.
simplify_optional_unions = True
simplify_optional_unions = False
typehints_defaults = None
typehints_default = 'braces'
typehints_defaults = 'comma'
typehints_default = 'braces-after'
Just noticed the following bug in the braces-after
option: when there's a multi-line parameter description, the default is placed at the end of the first line (at least with numpy style).
Poll assumes bug-free implementation 🙂
Hi @greschd,
I was now checking the bug you mentioned before and I am not sure I am getting the same behavior. This is a test I did with a very long parameter description:
Is that what you meant?
By the way, I am starting to change my mind about the option that I prefer... Now I see the comma
option a bit cleaner, with all the default values in the same place, right after the type:
I was now checking the bug you mentioned before and I am not sure I am getting the same behavior.
Doesn't look like it.. is the long parameter description split into multiple lines in the source for your example?
I was now checking the bug you mentioned before and I am not sure I am getting the same behavior.
Doesn't look like it.. is the long parameter description split into multiple lines in the source for your example?
My bad, that was the issue. I had all the description in a single line. If I split it into several lines I get the bug you mentioned:
Well, if you change your vote to the comma
option we can avoid the issue 😄
I have just updated my vote on the typehints_default
. I think I'd rather have all defaults in the same place with the comma
. IMO, it could get a bit messy when having different numbers of lines for each parameter when using braces-after
(plus, the fact that we avoid the aforementioned bug). Anyway, we can always change that.
@PProfizi, feel free to merge this PR if everything looks good to you (using typehints_defaults = "comma"
and simplify_optional_unions = False
).
Add example in plot_contour to verify that we are correctly handling typehinting.