Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class decorator version of renpy.register_statement #4846

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Gouvernathor
Copy link
Member

@Gouvernathor Gouvernathor commented Jul 16, 2023

There is some code duplication as it comes to the accepted parameters to register_statement, because the inspect module does that in py3 only. I suggest using it as soon as we pass to py3-only (the code has been left commented-out), or to wait until then for merging this.

The doc entry looks like this
image

Unless I add deco or decorator as a :doc: option, decorators can't be documented by docstring
@Gouvernathor Gouvernathor added the enhancement A proposed or requested enhancement or improvement to renpy's features. label Jul 16, 2023
@Booplicate
Copy link
Contributor

Wouldn't it make more sense to use an ABC with properly defined signature for this? That way you don't have to look up method names.

Also, while making a use out of magic methods looks interesting, it's not as obvious and goes against There should be one-- and preferably only one --obvious way to do it

@Gouvernathor
Copy link
Member Author

I don't see what the advantages of an ABC would be. What would be the defined signatures, self, lexer for either parse or __init__ ? All other parameters to register_statement are optional (save the name), so there could not be an abstract execute method, whether we call it that or __call__, or any other abstract method. So, if you meant API-wise I don't see an advantage, and if you meant implementation-wise I'm not sure what you have in mind to make it more efficient than this.

The class attribute/method names corresponding to the function's parameter seems obvious enough to me as a way to do it. As for the "only one" part, Python rarely even respects it itself. It's a bridge between allowing optimization for the class context, since using the constructor directly rather than a classmethod constructor saves a call but you can also make the class method return a different object if you want. As for the name, you need to have the class attribute way if you want to add statements with spaces in their names, yet if you don't it's nice not to have to set it manually. And execute, well __call__ does not have a similar functional bonus but it seems expected of something called "execute" - to execute something in programming you usually call it.

@Booplicate
Copy link
Contributor

I don't see what the advantages of an ABC would be

You'd know that it has to have parse and execute, for example.

Using __init__ and __call__ is odd because those are instance methods. It implies that you're actually creating those objects. Will __call__ actually receive the instance reference? Always? This idea looks more like a namespace than anything else. In that case parse and the others being static would make the most sense to me.

If renpy actually constructed the objects, then I'd understand why you'd use the magic methods.

@Gouvernathor
Copy link
Member Author

Gouvernathor commented Jul 16, 2023

The methods do what you tell them to. If you add a parse method (class or static, as I said in the doc), you may make it return an instance of the class, or not. In that case, Ren'Py doesn't create the object, you do. And if you don't add a parse method, the class constructor will be used which, by definition, returns an instance of the class.
As explained in the register_statement doc, the execute method will be passed what the parse method returned, so using __call__ only works if the object resulting of the parse was an instance of the class. That's why you can also have a class/static execute method, if you wanted to return another object (or a list of objects, for instance).
So, yes, you can use the class just as a namespace of static methods, but you can also use the class structure to reflect the fact that all methods apart from parse are passed the same parsed object so it makes sense for them to be methods of that object's class. But you don't have to, so that no possibilities you had with register_statement are closed when using register_decorator.

I believe you can register a statement without execute, if only because of the execute_init, execute_default and label methods. You can also make a statement like rpy python 3 which "executes" at parse time and leaves no trace in the AST - well, it does, but similar statements could afford not to I think - so it would not have either of these methods nor execute.

@mal
Copy link
Member

mal commented Jul 16, 2023

Is this worth adding to the core API? It's pretty trivial (and a good deal less "magic") for a creator to do this themselves if they want to use a class; directly passing the methods to register_statement, or should they really wish to, with their own decorator. The docs too already have an opinion on how to do this which works whether a creator is using functions or class methods, so I'm not sure this is worth the potential confusion.

@Gouvernathor
Copy link
Member Author

Gouvernathor commented Jul 17, 2023

I think there shouldn't be much confusion in that defining CDSs using a class seems like a much simpler recipe, in the general case, than creating a bunch of functions. So, we could have that as the primary way of creating CDS, and have register_statement as the "manual" low-level way. It may make CDS easier to make and understand for creators, with a redesign of the documentation for both functions by putting the decorator first and the function second - taking inspiration from Boop's idea of an abstract base class, in a way, documentation wise.

But it's also a very thin wrapper which only relies on documented things, so it may leave us with a maintenance burden for no major benefit. And it may also well live in a cookbook, as an add-on shared alongside renpy. I intended for it to be viable as that as well.

no need to edit the doc generation script actually
@mal
Copy link
Member

mal commented Jul 19, 2023

I'm not sure encouraging use of a class for this is a good idea. The reason to choose a class over a module is to retain and share state, something that these functions should not do.

@Gouvernathor Gouvernathor marked this pull request as draft July 21, 2023 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A proposed or requested enhancement or improvement to renpy's features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants