Opened 3 years ago

Closed 3 years ago

#23238 closed enhancement (fixed)

New syntax for GlobalOptions

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.0
Component: misc Keywords:
Cc: darij, dkrenn, andrew.mathas, tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 94807eb (Commits) Commit: 94807eb80a66f843222be674a47965ddc9dec798
Dependencies: Stopgaps:

Description (last modified by 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.

Change History (24)

comment:1 Changed 3 years ago by tscrim

  • Cc andrew.mathas tscrim added

comment:2 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 3 years ago by andrew.mathas

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.

comment:5 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Change syntax of GlobalOptions to New syntax for GlobalOptions

comment:6 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/change_syntax_of_globaloptions

comment:7 Changed 3 years ago by jdemeyer

  • Commit set to c17cbe3f345960289c7d0ba90f2b23b3e29e8430

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


New commits:

c17cbe3New class syntax for GlobalOptions

comment:8 Changed 3 years ago by jdemeyer

  • Dependencies set to #23277

comment:9 Changed 3 years ago by git

  • Commit changed from c17cbe3f345960289c7d0ba90f2b23b3e29e8430 to d8c1c2ffd2f0a9433ed0c7ea7792bd7ddee3e8c5

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

3be0ac2InstanceDocDescriptor: support __doc__ attribute on instance
d8c1c2fNew class syntax for GlobalOptions

comment:10 Changed 3 years ago by git

  • Commit changed from d8c1c2ffd2f0a9433ed0c7ea7792bd7ddee3e8c5 to 84a23634841d84093b6e6619b3b4544f1bb03ce3

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

84a2363New class syntax for GlobalOptions

comment:11 Changed 3 years ago by jdemeyer

  • Dependencies #23277 deleted
  • Description modified (diff)

comment:12 Changed 3 years ago by jdemeyer

  • Status changed from new to needs_review

comment:13 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:14 follow-up: Changed 3 years ago by tscrim

  • Status changed from needs_review to needs_work

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.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by 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.

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.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by tscrim

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.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 3 years ago by 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.

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?

comment:18 Changed 3 years ago by git

  • Commit changed from 84a23634841d84093b6e6619b3b4544f1bb03ce3 to aff9ee6a8cd550d76ff6bb6727960a25435f571f

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

aff9ee6Separate __doc__ examples and tests

comment:19 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by tscrim

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

comment:20 in reply to: ↑ 19 Changed 3 years ago by jdemeyer

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.

comment:21 Changed 3 years ago by git

  • Commit changed from aff9ee6a8cd550d76ff6bb6727960a25435f571f to 94807eb80a66f843222be674a47965ddc9dec798

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

94807ebUse "NAME" instead of "name"

comment:22 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:23 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM. Thank you.

comment:24 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/change_syntax_of_globaloptions to 94807eb80a66f843222be674a47965ddc9dec798
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.