New syntax for GlobalOptions

[jdemeyer]

Rethink the syntax of GlobalOptions. This is mainly meant to address the problem that the doc and end_doc attributes of a GlobalOptions object are not doctested (see #16693 and #18051 which were closed as duplicate of #14272 but which would better be fixed independently).

I am thinking of replacing

options = GlobalOptions('menu',
    doc='Fancy documentation\n'+'-'*19, end_doc='The END!',
    entree=dict(...), ...)

by

class options(GlobalOptions):
    """
    Fancy documentation
    -------------------

    @OPTIONS@

    The END!
    """
    name = "menu"
    entree = dict(...)

In other words, to use the class statement purely as a custom syntax, but without actually creating a class. The main advantage of this syntax is that doctests are just ordinary doctests, there is no need for doc= and end_doc= arguments.

This ticket is about enabling the new syntax in addition to the old syntax. Converting the existing use-cases of GlobalOptions will be delegated to follow-up tickets.

CC: @darijgr @dkrenn @AndrewAtLarge @tscrim

Component: misc

Author: Jeroen Demeyer

Branch/Commit: 94807eb

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/23238

[AndrewMathas]

comment:4

Not having the global options doc-tested seems to be causing problems, and all documentation should be doc-tested, so I think that this is a good idea. This said, I am surprised that there is testable code hidden in the global options that is not doc-tested elsewhere: tests for all methods should be given in the methods.

For what it's worth, my initial plan was to write the options as a class but one of the people I discussed this with thought that creating a class for each set of options was a little OTT.

[jdemeyer]

Branch: u/jdemeyer/change_syntax_of_globaloptions

[jdemeyer]

Commit: c17cbe3

[jdemeyer]

comment:7

This is an early prototype. Pickling is not yet supported and it needs a lot more tests and docs.

---

New commits:

c17cbe3

New class syntax for GlobalOptions

[jdemeyer]

Dependencies: #23277

[sagetrac-git]

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

3be0ac2	InstanceDocDescriptor: support `__doc__` attribute on instance
d8c1c2f	New class syntax for GlobalOptions

[sagetrac-git]

Changed commit from c17cbe3 to d8c1c2f

[sagetrac-git]

Changed commit from d8c1c2f to 84a2363

[sagetrac-git]

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

84a2363

New class syntax for GlobalOptions

[jdemeyer]

Changed dependencies from #23277 to none

[tscrim]

comment:14

Overall I like the improved way to create global options as classes. However, I do not like how name is treated as a special input; it looks too much like an option and someone could want an option called name. While this was not previously supported, it was clear from the class signature that this could not be an option. However, with the new format, it looks more like (black) magic that name is and should be treated differently. Perhaps just call it _name instead as that is the ultimate attribute it is suppose to be called. Moreover, Python does not distinguish between classes and instances (and global options are suppose to be unique class level attributes). At least, this feels a little more natural to me, even if it breaks encapsulation in a way.

I would prefer """ instead of ''', but I guess we cannot change that because of it being inside of a """ string and the doctesting framework (so we can't add an escape \).

I'm also not sure about the change to actually print the __doc__ as tests. I agree that it should be tested, but having to add all of those <BLANKLINE> takes away from the instructional value of the documentation.

[jdemeyer]

comment:15

Replying to @tscrim:

Moreover, Python does not distinguish between classes and instances (and global options are suppose to be unique class level attributes). At least, this feels a little more natural to me, even if it breaks encapsulation in a way.

I don't get what you mean with this.

I would prefer """ instead of ''', but I guess we cannot change that because of it being inside of a """ string and the doctesting framework (so we can't add an escape \).

We could escape \"\"\" I guess but that looks even uglier than '''.

I'm also not sure about the change to actually print the __doc__ as tests. I agree that it should be tested, but having to add all of those <BLANKLINE> takes away from the instructional value of the documentation.

Right. So maybe keep the doc as before but add tests too.

[tscrim]

comment:16

Replying to @jdemeyer:

Replying to @tscrim:

Moreover, Python does not distinguish between classes and instances (and global options are suppose to be unique class level attributes). At least, this feels a little more natural to me, even if it breaks encapsulation in a way.

I don't get what you mean with this.

Class attributes and instance attributes behave in exactly the same way:

sage: class Foo(object):
....:     bar = 5
....:     def __init__(self):
....:         self.var = -1
sage: f = Foo()
sage: f.bar
5
sage: f.var
-1
sage: f.bar = 2
sage: f2 = Foo()
sage: f2.bar
5
sage: f.bar
2

So having the class level attribute of _name is the same as setting _name in the __init__ (as far as the object is concerned). So it feels much more natural to me to be setting the name by using _name.

I would prefer """ instead of ''', but I guess we cannot change that because of it being inside of a """ string and the doctesting framework (so we can't add an escape \).

We could escape \"\"\" I guess but that looks even uglier than '''.

Exactly my conclusion. This comment ended up being more of a conversion with myself. Sorry, I should've added that I don't think we should change anything.

I'm also not sure about the change to actually print the __doc__ as tests. I agree that it should be tested, but having to add all of those <BLANKLINE> takes away from the instructional value of the documentation.

Right. So maybe keep the doc as before but add tests too.

I think this is the best way forward.

[jdemeyer]

comment:17

Replying to @tscrim:

So having the class level attribute of _name is the same as setting _name in the __init__ (as far as the object is concerned).

I see what you mean but I don't see the relevance to this ticket.

So it feels much more natural to me to be setting the name by using _name.

To me, the _name attribute on the instance is an implementation detail which is unrelated to the construction of the object. So I don't see it as an argument in favour of _name (but also not against it, it's just not relevant in my opinion).

I have also been considering NAME in all capitals. What do you think?

[sagetrac-git]

Changed commit from 84a2363 to aff9ee6

[sagetrac-git]

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

aff9ee6

Separate `__doc__` examples and tests

[tscrim]

comment:19

Replying to @jdemeyer:

Replying to @tscrim:

So having the class level attribute of _name is the same as setting _name in the __init__ (as far as the object is concerned).

I see what you mean but I don't see the relevance to this ticket.

Since this ticket is about setting the API for GlobalOptions and this will be used as part of the API, so I feel it is relevant.

So it feels much more natural to me to be setting the name by using _name.

To me, the _name attribute on the instance is an implementation detail which is unrelated to the construction of the object. So I don't see it as an argument in favour of _name (but also not against it, it's just not relevant in my opinion).

Well, this goes to the statement I made about breaking encapsulation, but I want to make it part of the API in a way. At least since we don't have an __init__ with a set signature (well, as much as Python can give).

I have also been considering NAME in all capitals. What do you think?

This also is good for me as it is clear it is special. I have a very mild preference for _name over NAME, but my opinion is not very strong (other than it not being name).

[jdemeyer]

comment:20

Replying to @tscrim:

Well, this goes to the statement I made about breaking encapsulation, but I want to make it part of the API in a way.

The _name attribute on the instance is a private implementation detail and explicitly not part of the API. This is compatible with the Python philosophy that names starting with underscores are private. Actually, thinking more about it, this is an argument against _name: the name/_name/NAME attribute when creating the class should be part of the API, so it should not start with an underscore.

Even if you consider the _name attribute on the instance as part of the API, most users won't care. So I don't see this as a reason to use the same name _name for constructing the class.

This also is good for me as it is clear it is special. I have a very mild preference for _name over NAME, but my opinion is not very strong (other than it not being name).

In that case I will use NAME.

[sagetrac-git]

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

94807eb

Use "NAME" instead of "name"

[sagetrac-git]

Changed commit from aff9ee6 to 94807eb

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:23

LGTM. Thank you.

[vbraun]

Changed branch from u/jdemeyer/change_syntax_of_globaloptions to 94807eb