Opened 3 years ago

Closed 3 years ago

#22611 closed enhancement (fixed)

Replace _sage_doc_ by a __doc__ descriptor

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.6
Component: documentation Keywords: days85
Cc: hivert, nthiery Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 5fe7cb0 (Commits) Commit: 5fe7cb01a8cd117ac6754c8301fb5ddf94d82813
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Currently, the method _sage_doc_ is used to implement custom documentation for instances of a class. Such a use case makes sense, but it should be implemented as the __doc__ attribute instead of the Sage-specific _sage_doc_.

I tried various ways of implementing this and I found several Cython bugs along the way. The current solution is not the cleanest, but it works in all cases (Cython and Python, extension types and normal classes).

In the cases where _sage_doc_ was implemented for a base class, this patch requires adding @instancedoc to every subclass. This is because Python never inherits the __doc__ attribute. There is a way around this using a metaclass, but I avoided that because metaclasses have their own problems (see #21681).

Change History (25)

comment:1 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Keywords days85 added

comment:2 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/22611

comment:3 Changed 3 years ago by jdemeyer

  • Cc hivert nthiery added
  • Commit set to 326465738e5a563ebffc86196f400829650cee63
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

3264657Replace _sage_doc_ by a __doc__ descriptor

comment:4 Changed 3 years ago by git

  • Commit changed from 326465738e5a563ebffc86196f400829650cee63 to fa42d8eb1966a7e1ade67bb40b42a80bb04059b1

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fa42d8eReplace _sage_doc_ by a __doc__ descriptor

comment:5 Changed 3 years ago by jdemeyer

There was an issue building the docs due to src/sage/parallel/decorate.py. I did not understand the problem but I fixed it anyway with

  • src/sage/parallel/decorate.py

    diff --git a/src/sage/parallel/decorate.py b/src/sage/parallel/decorate.py
    index ebc06e2..3305724 100644
    a b for a in args[0])) 
    284287            sage: sage_getdoc(p(f))
    285288            'Test docstring\n'
    286289        """
    287         from sage.misc.sageinspect import sage_getdoc
    288         return sage_getdoc(self.func)
     290        return self.func.__doc__
    289291
    290292
    291293def parallel(p_iter='fork', ncpus=None, **kwds):

comment:6 Changed 3 years ago by vdelecroix

In ticket description: and I was various Cython bugs along the way!?

comment:7 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 3 years ago by git

  • Commit changed from fa42d8eb1966a7e1ade67bb40b42a80bb04059b1 to 5eabe46ca0f929e8e09f0b90e295a0300afe3750

Branch pushed to git repo; I updated commit sha1. New commits:

5eabe46Make coverage script happy

comment:10 Changed 3 years ago by embray

This is just a stupid comment, but I'm curious about the naming--why @InstanceDoc and not @instance_doc? For whatever reason it's more common to name decorators with underscores instead of camel case (so much so that people will even go against the camel case convention for classes in cases where a decorator is implemented as a class).

Then again, maybe the existing naming nicely calls attention to the fact that this is intended as a class decorator.

comment:11 follow-ups: Changed 3 years ago by embray

What happens if _instancedoc_ is not defined? Will Cython make sure an AttributeError is raised in this case?

I think at the very least there should be a clear error message that _instancedoc_ must be defined (including on base classes that may not have a sensible _instancedoc_, in which case it might be good if the descriptor also handled NotImplemented errors).

comment:12 follow-up: Changed 3 years ago by embray

  • Reviewers set to Erik Bray

LGTM otherwise. Clever way to abuse the Python internals a bit, though I can't see anything inherently wrong with it, since tp_doc is suppose to be able to be NULL.

comment:13 in reply to: ↑ 12 Changed 3 years ago by jdemeyer

Replying to embray:

LGTM otherwise. Clever way to abuse the Python internals a bit

Like I say on the ticket description, this is the only way to make it work for all classes. Any pure Python implementation wouldn't work for extension types.

comment:14 in reply to: ↑ 11 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to embray:

What happens if _instancedoc_ is not defined? Will Cython make sure an AttributeError is raised in this case?

Yes, AttributeError works the same way in Cython.

comment:15 in reply to: ↑ 11 Changed 3 years ago by jdemeyer

Replying to embray:

What happens if _instancedoc_ is not defined? Will Cython make sure an AttributeError is raised in this case?

I think at the very least there should be a clear error message that _instancedoc_ must be defined

You mean an error which is not just AttributeError: 'foo' object has no attribute '_instancedoc_'?

comment:16 Changed 3 years ago by embray

Right. For example, in classes with the ABCMeta metaclass, if there are any unimplemented abstractproperty or abstractmethod, trying to instantiate the class results in:

TypeError: Can't instantiate abstract class A with abstract methods foo

This is a slightly different case of course, since it's an error that would occur at class definition time. I was just using this as an example of a clearer error message.

comment:17 Changed 3 years ago by git

  • Commit changed from 5eabe46ca0f929e8e09f0b90e295a0300afe3750 to 676a80428a980b326a936a42759ff3ba88398f27

Branch pushed to git repo; I updated commit sha1. New commits:

f92c258Doctest an example where _instancedoc_ is not defined
676a804Rename InstanceDoc -> instancedoc

comment:18 Changed 3 years ago by jdemeyer

I replaced InstanceDoc with instancedoc (I didn't use instance_doc just because the method name _instancedoc_ doesn't have an underscore either. And anyway, Python uses staticmethod and not static_method.)

comment:19 Changed 3 years ago by embray

I still don't think the AttributeError is very useful, especially considering that you're getting an exception from a builtin module and can't see in the traceback what's actually causing it. Of course, if one is implementing a class using @instancedoc this should be expected, but it still might seem mysterious, I think, compared to a slightly customized message (it could still be an AttributeError, though TypeError is used more commonly for cases where an expected interface is not found--e.g. len() on an object with no __len__.).

comment:20 Changed 3 years ago by git

  • Commit changed from 676a80428a980b326a936a42759ff3ba88398f27 to 5fe7cb01a8cd117ac6754c8301fb5ddf94d82813

Branch pushed to git repo; I updated commit sha1. New commits:

5fe7cb0Change exception message for missing _instancedoc_

comment:21 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Here you go...

comment:22 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

Great, I think that's a lot nicer! Thanks for this--honestly this is so useful I think some form of this should be implemented in Python. I might suggest that, actually, since I see the question of per-instance docstrings come up from time to time, with no good solution. If some kind of __instancedoc__ special method were built into Python it would obviate the need for the special descriptor or the decorator.

Last edited 3 years ago by embray (previous) (diff)

comment:23 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:24 Changed 3 years ago by jdemeyer

Thanks!

comment:25 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/22611 to 5fe7cb01a8cd117ac6754c8301fb5ddf94d82813
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.