Test whether function should use Yields instead of Returns#7258
Test whether function should use Yields instead of Returns#7258eriknw wants to merge 23 commits intonetworkx:mainfrom
Conversation
|
When would this "test" get run? Not during CI? I guess this means that for each new generator added, we should update this list in this file? |
|
This currently gets tested during the "test / dispatch" CI run. It actually inspects the return type. And, yeah, as docs get updated, the sets will need updated too. The error message will tell you what to do.
|
|
I see -- this is just a list of the function with docs that have it wrong... :) |
|
I went through the exercise of testing out what the numpydoc validation checks would say about this by turning them on during the sphinx build. This can be achieved by adding the following two lines1 to the numpydoc_validation_checks = {"YD01"}
numpydoc_validation_exclude = [r".coreviews."]Running the docbuild then spits out an additional 94 warnings - see details below Numpydoc validation log: return should be yieldWARNING: [numpydoc] Validation warnings while processing docstring for 'networkx.algorithms.assortativity.node_attribute_xy': YD01: No Yields section found The conclusion I draw from the above experiment is that numpydoc validation is sufficient to catch these errors. The course of action that I'd propose is to:
Footnotes
|
|
Thanks for running that experiment @rossbar. I'm glad to be able to compare.
I think that's the wrong conclusion. First, your count is wrong. It found 47, not 94 (it reports each one twice?). Second, it appears to miss about a two-thirds of the functions identified in this PR, so it actually doesn't do a very good job (why not?!): TBH, I would expect it to find more. Third, it's a more relaxed check than done in this PR. It looks if "Yields" is missing regardless if "Returns" is present. This PR could trivially be updated to also check for no "Yields" regardless of "Returns". However, I think it's less work (and more important) to change "Returns" to "Yields" vs. adding a new "Yields" section, but of course the latter could be done too. |
|
Actually, I take back what I said. There is a difference between yielding and returning an iterator. Consider: def f1():
raise ValueError
yield from [1, 2]
def f2():
raise ValueError
yield 1
yield 2
def f3():
raise ValueError
return iter([1, 2])
f1()
f2()
f3()Do you know what happens? So, there is a difference whether we check |
|
Alright, I grouped the function names by whether they are generator functions, return generators, or return iterators. Generator functions is the largest group and should definitely use "Yields" (also, numpydoc validation misses a lot of these). |
Thanks @eriknw for checking this, I never actually compared to the list here 🙃 . Indeed you're right, there are quite a few generators missed by the numpydoc validation. Diving a little deeper --- the numpydoc validation is implemented with >>> import inspect
>>> import networkx as nx
>>> inspect.isgeneratorfunction(nx.all_triads) # Expect true
FalseI think the decorators are the culprit. A simple test is to define an un-decorated version of >>> def mytriads(G):
...: triplets = combinations(G.nodes(), 3)
...: for triplet in triplets:
...: yield G.subgraph(triplet).copy()
>>> inspect.isgeneratorfunction(mytriads)
TrueNow, why this is happening for some of these functions but not all (since everything in NetworkX is decorated with
Agreed - I think "Yields" is appropriate even in the |
|
👍 It looks like Perhaps numpydoc could be updated to unwrap |
This is an issue in PR form (but maybe it could be merged since it serves as a test?)
This is a more thorough report (as a followup to #7252 and #7253) of functions that perhaps ought to use
Yieldsinstead ofReturnsin their docstrings. I actually don't know the best recommended practice here--is there a difference whether a function uses e.g.for x in spam: yield xoryield from spamcompared to e.g.return (x for x in spam)?Context: I was poking around return types in #7191, and it would have been helpful to easily know (using grep) which functions return iterators.