BUG: Defer to autodoc for signatures #221

thequackdaddy · 2019-05-29T05:04:05Z

xref #220

I think this should pass. It fails because method signatures include self.

I'll be happy to help make this pass.

jnothman · 2019-06-01T14:19:32Z

Why do you think this needs to be an option?

thequackdaddy · 2019-06-01T14:21:18Z

I don't. I just don't know if you want to defer this to autodoc. (More of an architecture question.)

Could just delete all that code in that case.

Throwing out ideas.

thequackdaddy · 2019-06-01T14:26:03Z

I should have mentioned, it appears that the numpydoc way of getting signatures on class method is inconsistent between python 2 and python 3. Maybe another reason to just say, "screw it... use autodoc signatures".

But I don't want to upset the apple cart.

thequackdaddy · 2019-06-01T14:26:40Z

(and who builds docs on python 2 anyway)

bashtage · 2019-08-01T17:40:43Z

I build latest statsmodels doc with this branch and it look good. Handles both function sigs in docstrings (e.g., from Cython) and removes self when needed. For example, (OLS)

larsoner · 2019-08-01T19:37:20Z

Maybe another reason to just say, "screw it... use autodoc signatures".

But I don't want to upset the apple cart.

Do tests pass if you always use autodoc? It would be nice to only have one code path if possible, especially if we know that the non-autodoc one is buggy.

thequackdaddy · 2019-08-01T21:52:04Z

@larsoner I honestly haven't tried that. I'll try to get to it in the next few days...

thequackdaddy · 2019-08-01T23:32:07Z

@larsoner So it was much easier than I thought...

Answer is that if I remove this code, we should revert to auto doc.

numpydoc/numpydoc/docscrape.py

Lines 575 to 590 in b45eb2b

    
           if not self['Signature'] and func is not None: 
        
               func, func_name = self.get_func() 
        
               try: 
        
                   try: 
        
                       signature = str(inspect.signature(func)) 
        
                   except (AttributeError, ValueError): 
        
                       # try to read signature, backward compat for older Python 
        
                       if sys.version_info[0] >= 3: 
        
                           argspec = inspect.getfullargspec(func) 
        
                       else: 
        
                           argspec = inspect.getargspec(func) 
        
                       signature = inspect.formatargspec(*argspec) 
        
                   signature = '%s%s' % (func_name, signature.replace('*', r'\*')) 
        
               except TypeError: 
        
                   signature = '%s()' % func_name 
        
               self['Signature'] = signature

The only induced failure is this one...

numpydoc/numpydoc/tests/test_docscrape.py

Lines 723 to 731 in b45eb2b

    
           def test_escape_stars(): 
        
               signature = str(doc3).split('\n')[0] 
        
               assert signature == r'my_signature(\*params, \*\*kwds)' 
        
               def my_func(a, b, **kwargs): 
        
                   pass 
        
               fdoc = FunctionDoc(func=my_func) 
        
               assert fdoc['Signature'] == r'my_func(a, b, \*\*kwargs)'

larsoner · 2019-08-01T23:59:52Z

numpydoc/tests/test_docscrape.py

-        pass
-
-    fdoc = FunctionDoc(func=my_func)
-    assert fdoc['Signature'] == r'my_func(a, b, \*\*kwargs)'


This might actually be important. Do things with *args, **kwargs render properly now? That's what this is meant to check/ensure. Can we replace this test rather than just remove it? Maybe check that the RST generated has the *s escaped properly?

larsoner

The other thing I wonder about is that git grep Signature has more hits in numpydoc than just the ones removed here. is self['Signature'] now created by autodoc automatically so we don't need to worry about this? Or is self['Signature'] always None after this PR so that logic elsewhere like the following could be simplified?

numpydoc/numpydoc.py:    sig = doc['Signature'] or getattr(obj, '__text_signature__', None)

thequackdaddy · 2019-08-02T00:08:03Z

Would you be ok if I made a PR to turn on coveralls codecov? Might be easier to see what's really being tested and where.

larsoner · 2019-08-02T02:51:46Z

Sure, they're a bit buggy but we can configure then to be useful

bashtage · 2019-08-02T07:59:40Z

Published statsmodels devel docs using @thequackdaddy 's branch. **kwargs looks fine.

https://www.statsmodels.org/devel/generated/statsmodels.tsa.statespace.dynamic_factor.DynamicFactor.html?highlight=dynamicfactor#statsmodels.tsa.statespace.dynamic_factor.DynamicFactor

larsoner · 2019-08-09T13:01:46Z

@thequackdaddy feel free to rebase now that #230 is in and then hopefully you can change the lines marked XXX here:

https://github.com/numpy/numpydoc/pull/230/files#diff-6624c1bee5f6adf586fc42bb69eb40b6R48-R54

thequackdaddy · 2019-08-10T13:08:24Z

Rebase is done and I think that might be all we need. Thoughts?

larsoner

Looks good to me, +1 for merge. Would be good to get another set of eyes on it, though, in case I'm missing something about why we had that code in there in the first place. It is about 10 years old, though, so maybe its just there to work around something that has been fixed nowadays.

jnothman · 2019-08-18T11:49:17Z

Please update the PR title. It certainly looks like a simple solution.

I'm building scikit-learn docs with this for review at scikit-learn/scikit-learn#14681

thequackdaddy · 2019-08-18T16:27:20Z

@jnothman I changed the title.

jnothman · 2019-08-18T23:29:29Z

I like how this looks in the member methods listing (autosummary). It also affects the signatures shown in method documentation. Here I see things like get_params(self, deep=True) become get_params(deep=True) which is fine, and __init__(self, /, *args, **kwargs) become __init__($self, /, *args, **kwargs) in some cases (I'm seeing it in sklearn.base, like here but not elsewhere like here). Any idea what this $ is about? More $ here

gforsyth · 2020-03-14T00:19:00Z

@seberg -- this PR does fix the issues we were discussing at the dask developers meeting. If this can be merged and a new release cut then we can unpin numpydoc in a bunch of downstream projects that are affected by the double-escaping we were seeing in autosummary.

jnothman · 2020-03-16T07:16:29Z

@gforsyth, am I alone in getting those $s rendering, per my comment above (#221 (comment))? I don't know why they are appearing...

gforsyth · 2020-03-16T17:44:50Z

Hey @jnothman -- I grepd over some rendered documentation to look for extra $s and I haven't come across any when building dask or distributed docs. I can try to build sklearn later today and see if I hit the same issue.

thequackdaddy · 2020-06-23T09:50:58Z

Is there any chance of $module (probably not, mostly built-ins or lambdas) or $type here?

Easy to check for that as well and replace.

numpydoc/numpydoc.py

larsoner

Just a minor simplification idea, otherwise LGTM

numpydoc/numpydoc.py

larsoner

LGTM +1 for merge.

@bashtage or @gforsyth do you want to try it out and/or review? @jnothman you might also be interested in looking again

@thequackdaddy if nobody complains in a few days feel free to ping me again, I'll run some final tests locally and merge

gforsyth

Changes look good and I've confirmed that building docs for dask/dask works as expected. Thanks for pushing on this!

jnothman

Thanks

numpydoc/numpydoc.py

numpydoc/tests/test_numpydoc.py

jnothman · 2020-06-25T21:23:09Z

numpydoc/tests/test_numpydoc.py

+    assert (_clean_text_signature('func($self, foo="$self")')
+            == 'func(foo="$self")')
+    assert _clean_text_signature('func(self, other)') == 'func(self, other)'
+    assert _clean_text_signature('func(self, $self)') == 'func(self)'


I'm not sure this is a thing :)

jnothman · 2020-06-25T21:28:21Z

numpydoc/numpydoc.py

+    sig = sig[end:-1]
+    params = sig.split(', ')
+    for param in ('$self', '$module', '$type', '/'):
+        if param in params:


I don't think it'll happen often, but I'm still not comfortable with the fact that this processes all parts of the sig. Can't we check explicitly that the / precedes a *, and that the $ is at the beginning, rather than use the split and join logic?

sig = re.sub(r'^\$(self|module|type), ', '', sig, count=1) sig = re.sub(r', /, \*', ', *', sig, count=1)

Is this not sufficient?

I'll confess that I have (almost) no understanding of how these C text signature things work.

What you have would make it so func($self) would still be func($self). Is that alright? I have no idea what the possible syntax is for this.

If you (or someone else) has some kind of documentation on what is possible, that would make this back-and-forth much easier.

Sorry, I meant to apply those substitutions after

start_pattern = re.compile(r"^[^(]*\(") start, end = start_pattern.search(sig).span() start_sig = sig[start:end] sig = sig[end:-1]

They're just a bit more careful than splitting on every comma.

Actually I think this makes sense. Let me change that.

To handle func($self), I guess you could just add a ’, ‘ right before your code. Then use regex to strip off a trailing ’, ‘ if it exists.

Do you think that would work?

Sorry, didn't handle that case, did I?
Can use r'^\$(self|module|type)(, |$))?' which will strip a comma, or check that we're at the end.

Take a peek at this. I had to do something to check whether the / was the start of the string or preceded by a , .

jnothman

The output looks good for Scikit-learn, and I think this is safer than it was yesterday, so I'm okay with it.

jnothman · 2020-06-26T00:45:58Z

numpydoc/numpydoc.py

+    sig = sig[end:-1]
+    params = sig.split(', ')
+    for param in ('$self', '$module', '$type', '/'):
+        if param in params:


Sorry, I meant to apply those substitutions after

start_pattern = re.compile(r"^[^(]*\(") start, end = start_pattern.search(sig).span() start_sig = sig[start:end] sig = sig[end:-1]

They're just a bit more careful than splitting on every comma.

numpydoc/numpydoc.py

jnothman · 2020-06-26T02:13:27Z

So one last thing: I think this now stops displaying self, cls, etc. where it used to in method docs.

I suspect we should at least document this in a change log.

thequackdaddy · 2020-06-26T02:22:35Z

So one last thing: I think this now stops displaying self, cls, etc. where it used to in method docs.

I suspect we should at least document this in a change log.

It looks like this project generally builds changelogs as part of the release cycle, unless I'm missing something? I just see a release notes file that looks like it was built for the 1.0 release. I don't see where I can edit in that?

thequackdaddy · 2020-06-26T02:25:10Z

So one last thing: I think this now stops displaying self, cls, etc. where it used to in method docs.
I suspect we should at least document this in a change log.

It looks like this project generally builds changelogs as part of the release cycle, unless I'm missing something? I just see a release notes file that looks like it was built for the 1.0 release. I don't see where I can edit in that?

Never mind. I guess the file is meant to be cumulative. I'll give it the old college try.

thequackdaddy · 2020-06-26T13:07:24Z

Is everyone content?

jnothman · 2020-06-27T11:03:03Z

Thank you @thequackdaddy!

thequackdaddy changed the title ~~TST: Desired test~~ ENH: Allow user to say whether numpydoc or autodoc should create signatures. Jun 1, 2019

thequackdaddy mentioned this pull request Jun 1, 2019

self included in list of params for method #220

Closed

larsoner reviewed Aug 1, 2019

View reviewed changes

larsoner reviewed Aug 2, 2019

View reviewed changes

larsoner mentioned this pull request Aug 2, 2019

ENH: Test full output and coverage #230

Merged

larsoner approved these changes Aug 12, 2019

View reviewed changes

jnothman mentioned this pull request Aug 18, 2019

[NOMRG] Try docs with numpydoc#221 scikit-learn/scikit-learn#14681

Closed

thequackdaddy changed the title ~~ENH: Allow user to say whether numpydoc or autodoc should create signatures.~~ BUG: Defer to autudoc for signatures Aug 23, 2019

larsoner mentioned this pull request Aug 25, 2019

DOC: bad formatting of *args, **kwargs in autosummary scipy/scipy#10723

Closed

rth mentioned this pull request Oct 10, 2019

Python 2 / 3.4 end of support timeline #235

Closed

gforsyth mentioned this pull request Mar 14, 2020

Pin numpydoc to avoid doubly-escaped * dask/distributed#3530

Merged

Fix python C sigangure $ issues. Revert removal of star test.

d2b943e

bashtage reviewed Jun 23, 2020

View reviewed changes

numpydoc/numpydoc.py Outdated Show resolved Hide resolved

Add helper funciton to clean-up C __text_signatures__

3c6d16d

larsoner reviewed Jun 23, 2020

View reviewed changes

numpydoc/numpydoc.py Outdated Show resolved Hide resolved

larsoner approved these changes Jun 23, 2020

View reviewed changes

PEP-8 fixes and readability

aa2189d

gforsyth approved these changes Jun 24, 2020

View reviewed changes

jnothman reviewed Jun 25, 2020

View reviewed changes

numpydoc/numpydoc.py Outdated Show resolved Hide resolved

numpydoc/numpydoc.py Outdated Show resolved Hide resolved

numpydoc/numpydoc.py Outdated Show resolved Hide resolved

numpydoc/tests/test_numpydoc.py Show resolved Hide resolved

thequackdaddy requested a review from jnothman June 25, 2020 14:52

Additional tests and less-strict space checking

339cf8a

jnothman reviewed Jun 25, 2020

View reviewed changes

jnothman approved these changes Jun 26, 2020

View reviewed changes

jnothman reviewed Jun 26, 2020

View reviewed changes

numpydoc/numpydoc.py Outdated Show resolved Hide resolved

Use more gentle regexp to remove $ and forward slash

5df5f28

DOC: Add change to release notes [skip ci]

88aaa5f

thequackdaddy changed the title ~~BUG: Defer to autudoc for signatures~~ BUG: Defer to autodoc for signatures Jun 26, 2020

jnothman approved these changes Jun 27, 2020

View reviewed changes

jnothman merged commit 37e4f25 into numpy:master Jun 27, 2020

thequackdaddy deleted the getdoc branch June 27, 2020 13:05

This was referenced Jun 27, 2020

DOC: Numpydoc signatures statsmodels/statsmodels#6825

Merged

New release #270

Closed

larsoner added the type: Enhancement label Jul 1, 2020

m-melis mentioned this pull request Jul 3, 2020

version 1.1.0 failing at _clean_text_signature with TypeError: expected string or bytes-like object #276

Open

larsoner mentioned this pull request Aug 2, 2020

Epochs: Baseline-Default problems mne-tools/mne-python#8078

Closed

BUG: Defer to autodoc for signatures #221

BUG: Defer to autodoc for signatures #221

Conversation

thequackdaddy commented May 29, 2019 • edited Loading

jnothman commented Jun 1, 2019

thequackdaddy commented Jun 1, 2019 • edited Loading

thequackdaddy commented Jun 1, 2019

thequackdaddy commented Jun 1, 2019

bashtage commented Aug 1, 2019

larsoner commented Aug 1, 2019

thequackdaddy commented Aug 1, 2019

thequackdaddy commented Aug 1, 2019

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

thequackdaddy commented Aug 2, 2019

larsoner commented Aug 2, 2019

bashtage commented Aug 2, 2019

larsoner commented Aug 9, 2019

thequackdaddy commented Aug 10, 2019

larsoner left a comment

Choose a reason for hiding this comment

jnothman commented Aug 18, 2019

thequackdaddy commented Aug 18, 2019

jnothman commented Aug 18, 2019

gforsyth commented Mar 14, 2020

jnothman commented Mar 16, 2020

gforsyth commented Mar 16, 2020

thequackdaddy commented Jun 23, 2020

larsoner left a comment

Choose a reason for hiding this comment

larsoner left a comment

Choose a reason for hiding this comment

gforsyth left a comment

Choose a reason for hiding this comment

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Jun 26, 2020

thequackdaddy commented Jun 26, 2020

thequackdaddy commented Jun 26, 2020

thequackdaddy commented Jun 26, 2020

jnothman commented Jun 27, 2020

thequackdaddy commented May 29, 2019 •

edited

Loading

thequackdaddy commented Jun 1, 2019 •

edited

Loading