-
Notifications
You must be signed in to change notification settings - Fork 135
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
correct create_personality function params to reflect usage #375
correct create_personality function params to reflect usage #375
Conversation
Broken tests appear unrelated. Local build of docs succeeds. |
docs/source/plug-in.md
Outdated
@@ -31,7 +31,7 @@ class TemplatePersonality(LoggingConfigurable): | |||
been specified.""" | |||
pass | |||
|
|||
def create_personality(self, parent): | |||
def create_personality(parent, log): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about the usage of *args, **kwargs
here or in the existing declarations: Based on init_configurables
, I'm not I'm not seeing how any additional args could be passed to create_personality
via configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we may find the need to convey other information to a "personality", at which time we'd add a parameter (e.g., foo=bar
) in init_configurables
. Personalities that don't know about the parameter could still be called (and ignore it), while those that know what foo
is for would use it via kwargs["foo"]
, for example.
This, coupled with the idea that signatures for "custom personalities" should match that of the two existing personalities, leads me to argue for using *args, **kwargs
due to precedent.
One of the things that makes this confusing is the lack of initializers with "Configurable" classes - on which most of Jupyter is based. Both parent
and log
are "well-known" parameters within the LoggingConfigurable
class hierarchy. (Not sure if this helps or not.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to use that for convention and extensibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - sorry about the CI. This project needs substantial maintenance!
The names of the positions args used in the docs don't reflect usage; this leads to a
TypeError
. This change address that issue.