MAINT: add typehint example #169

GuillemBarroso · 2022-09-27T15:32:16Z

Add example in plot_contour to verify that we are correctly handling typehinting.

codecov · 2022-09-27T15:40:22Z

Codecov Report

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

GuillemBarroso · 2022-09-27T17:51:11Z

Hi @PProfizi,

I've added an actual example of a method with typehinting: plot_contourmethod in ResultData class. It is finally showing the types without having to specify them in the method's description.

Changes description:

result_data.py: I have removed the types in the description. Note that "optional" cannot be placed next to the parameter! That was the reason for the docs not showing the types! I have added the word optional in the description of the parameter.
requirements_docs.txt: I have forced a compatible version of sphinx-autodoc-typehints.
conf.py: I have added the autodoc_typehints parameter to show types in the description
.gitignore: I am ignoring some files that appear when generating the documentation locally.

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.

.gitignore

PProfizi · 2022-09-28T08:55:12Z

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.

GuillemBarroso · 2022-09-28T18:00:25Z

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.

For the default sphinx-autodoc-typehints configuration (typehints_defaults = None and simplify_optional_unions = True) we get:

For the above proposed parameter combination sphinx-autodoc-typehints configuration (typehints_defaults = comma and simplify_optional_unions = True) we get:

For the above proposed parameter combination sphinx-autodoc-typehints configuration (typehints_defaults = braces and simplify_optional_unions = False) we get:

For the above proposed parameter combination 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.

MaxJPRey · 2022-09-29T09:28:29Z

@RobPasMue You could be interested in that for your geometry project.

MaxJPRey · 2022-09-29T09:37:43Z

Adding @akaszynski because it could be used for other projects.

greschd · 2022-09-29T10:21:39Z

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.

MaxJPRey · 2022-09-29T10:44:55Z

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?

GuillemBarroso · 2022-09-29T12:33:07Z

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?

Done. Please, see comment above.

RobPasMue · 2022-09-29T12:45:06Z

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 =)

PProfizi · 2022-09-29T13:06:39Z

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.

GuillemBarroso · 2022-09-29T13:09:09Z

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.

PProfizi · 2022-09-29T13:41:49Z

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.

jorgepiloto · 2022-09-29T15:57:06Z

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 numpy/numpydoc#196

greschd · 2022-09-29T19:21:20Z

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.

GuillemBarroso · 2022-09-30T14:06:14Z

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 ansys/pyansys-dev-guide#187.

I think this is now ready to merge.

greschd · 2022-09-30T14:24:36Z

Let's make a poll here: Upvote your preferred option.

greschd · 2022-09-30T14:24:50Z

simplify_optional_unions = True

greschd · 2022-09-30T14:25:00Z

simplify_optional_unions = False

greschd · 2022-09-30T14:25:15Z

typehints_defaults = None

greschd · 2022-09-30T14:26:03Z

typehints_default = 'braces'

greschd · 2022-09-30T14:26:14Z

typehints_defaults = 'comma'

greschd · 2022-09-30T14:26:26Z

typehints_default = 'braces-after'

greschd · 2022-09-30T14:31:25Z

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 🙂

PProfizi · 2022-09-30T14:50:40Z

src/ansys/dpf/post/result_data.py

-    def plot_contour(self, display_option: str = "time", option_id=1, **kwargs):
+    def plot_contour(
+        self,
+        display_option: Optional[str] = "time",


@GuillemBarroso quick question: is Optional mandatory even when it obviously is due to the default value?

It's not mandatory! I was just trying to follow the same criteria. You can add Optional whenever you want. For instance, this source code:

Renders as:

Is that what you meant?

Common misunderstanding here: Optional does not mean "you do not have to pass this parameter". It means "this parameter can be None". It's optional in the type-theory sense, not optional in the "has a default" sense.

Example:

The following is typing.Optional

def func(x: Optional[int]) -> int: if x is None: return 0 else: return x + 1

This is optional in the "has a default" sense:

def func(x: int = 0) -> int: return x + 1

The typing.Optional can sometimes be a bit of a code smell (better to give a nice default value!). And it's wrong to mark a parameter as Optional if passing None to it will not be handled gracefully, even if it has a default value.

See also: https://docs.python.org/3/library/typing.html#typing.Optional

True! That's what we discussed during last meeting. I have amended the method's description without typing.Optional so it now renders as:

I will change the optional parameters depending on the poll's result.

I will change the optional parameters depending on the poll's result.

If braces-after ends up winning, we should first figure out if we can get around the aforementioned bug.

GuillemBarroso · 2022-10-03T08:45:40Z

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:

greschd · 2022-10-03T10:11:52Z

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?

GuillemBarroso · 2022-10-03T10:16:52Z

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:

greschd · 2022-10-03T10:46:30Z

Well, if you change your vote to the comma option we can avoid the issue 😄

GuillemBarroso · 2022-10-04T15:56:55Z

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).

See ansys/pydpf-post#169

add typehint in plot_contour to test extension

178de84

GuillemBarroso added the maintenance Repository structure maintenance label Sep 27, 2022

GuillemBarroso added 3 commits September 27, 2022 17:48

add autodoc_typehints variable

e8ce708

follow exact notation in sphinx-autodoc-typehints documentation

9c838a2

working version when docs generated locally

7384392

GuillemBarroso mentioned this pull request Sep 28, 2022

Typehinting extensions proposed may not work ansys/pyansys-dev-guide#187

Open

PProfizi requested changes Sep 28, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

RobPasMue mentioned this pull request Sep 29, 2022

Improve API reference in the docs ansys/pyansys-geometry#143

Closed

2 tasks

GuillemBarroso added 2 commits September 30, 2022 15:53

typehints example in plot_contour

c416d86

fix style

02bb1c8

GuillemBarroso requested a review from PProfizi September 30, 2022 14:06

PProfizi approved these changes Sep 30, 2022

View reviewed changes

GuillemBarroso added 2 commits September 30, 2022 17:46

remove Optional

01e4536

remove Optional

0628339

Merge branch 'master' into maint/typehint-example

2fab5b1

PProfizi merged commit 518d7bf into master Oct 5, 2022

PProfizi deleted the maint/typehint-example branch October 5, 2022 08:36

greschd added a commit to ansys/pyacp that referenced this pull request Oct 6, 2022

Adopt common autodoc_typehints configuration

23ebb58

See ansys/pydpf-post#169

PProfizi mentioned this pull request Jan 2, 2023

Edits to DPF-Core RST and TXT files ansys/pydpf-core#729

Merged

MAINT: add typehint example #169

MAINT: add typehint example #169

Conversation

GuillemBarroso commented Sep 27, 2022

codecov bot commented Sep 27, 2022 • edited Loading

Codecov Report

GuillemBarroso commented Sep 27, 2022

PProfizi commented Sep 28, 2022 • edited Loading

GuillemBarroso commented Sep 28, 2022 • edited Loading

MaxJPRey commented Sep 29, 2022

MaxJPRey commented Sep 29, 2022

greschd commented Sep 29, 2022

MaxJPRey commented Sep 29, 2022

GuillemBarroso commented Sep 29, 2022

RobPasMue commented Sep 29, 2022

PProfizi commented Sep 29, 2022 • edited Loading

GuillemBarroso commented Sep 29, 2022

PProfizi commented Sep 29, 2022

jorgepiloto commented Sep 29, 2022 • edited Loading

greschd commented Sep 29, 2022

GuillemBarroso commented Sep 30, 2022

greschd commented Sep 30, 2022

greschd commented Sep 30, 2022

greschd commented Sep 30, 2022

greschd commented Sep 30, 2022

greschd commented Sep 30, 2022

greschd commented Sep 30, 2022

greschd commented Sep 30, 2022

greschd commented Sep 30, 2022

PProfizi Sep 30, 2022

Choose a reason for hiding this comment

GuillemBarroso Sep 30, 2022

Choose a reason for hiding this comment

greschd Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

greschd Sep 30, 2022

Choose a reason for hiding this comment

GuillemBarroso Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

greschd Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

GuillemBarroso commented Oct 3, 2022 • edited Loading

greschd commented Oct 3, 2022

GuillemBarroso commented Oct 3, 2022

greschd commented Oct 3, 2022

GuillemBarroso commented Oct 4, 2022

codecov bot commented Sep 27, 2022 •

edited

Loading

PProfizi commented Sep 28, 2022 •

edited

Loading

GuillemBarroso commented Sep 28, 2022 •

edited

Loading

PProfizi commented Sep 29, 2022 •

edited

Loading

jorgepiloto commented Sep 29, 2022 •

edited

Loading

greschd Sep 30, 2022 •

edited

Loading

GuillemBarroso Sep 30, 2022 •

edited

Loading

greschd Sep 30, 2022 •

edited

Loading

GuillemBarroso commented Oct 3, 2022 •

edited

Loading