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

IDE support #276

Merged
merged 15 commits into from
Aug 1, 2018
Merged

IDE support #276

merged 15 commits into from
Aug 1, 2018

Conversation

rmarren1
Copy link
Contributor

This builds on #271 and adds explicit arguments to generated dash component python class __init__ functions (rather than **kwargs), which improves experience in an IDE.

Atom example

atom

Pycharm example

pycharm

Jupyter Notebook example

jupyter

Notes
  • If dash_html_components or dash_core_components are updated build using this version of the dash module, a user must also update dash since a new function _explicitize_args is added to dash.development.base_component. There are workarounds for this (putting the new function in the archetype generator or into the init.py file, or any shared file in a component module), but I think they are messy since it separates base component code into multiple repositories. I will add an exception to both of the dash-*-components packages that say to download the new dash version.
  • Autocomplete functions which rely on the inspect would not work for python2. They work for python 3 since we are able to change the function signature of Component.__init__ to be correct even though it is actually (*args, **kwargs) after the _explicitize_args decorator is applied. This did not matter for Atom and PyCharm, but it seems Jupyter uses the inspect module to perform autocomplete.
  • Installing this new dash version and keeping old dash_*_components modules auto-magically adds the auto-complete functionality to Jupyter notebooks, but not Atom or PyCharm.

@rmarren1
Copy link
Contributor Author

rmarren1 commented Jul 4, 2018

I've thought about the solution we discussed last stand-up, which would look something like this in a dash component

from dash.development.base_component import Component

DEFAULT = object()

class Button(Component):
    """docstring...'"""
    def __init__(self, children=DEFAULT, id=DEFAULT, n_clicks=DEFAULT, ...):

and the current solution in this PR looks like this:

from dash.development.base_component import Component, _explicitize_args


class Button(Component):
    docstirng...
    @_explicitize_args
    def __init__(self, children=None, id=None, n_clicks=None, ..., **kwargs):
        ...
        _explicit_args = kwargs.pop('_explicit_args')
        _locals = locals()
        _locals.update(kwargs)  # For wildcard attrs
        args = {k: _locals[k] for k in _explicit_args if k != 'children'}
        ...

the _explicitize_args decorator will check which arguments were set explicitly by the user and add those to a list _explicit_args which is sent also to the __init__ function.

The object based solution has two benefits over the solution I have implemented in this PR,

  1. No new code (the _explicitize_args function) is introduced into the base component, so users who upgrade dash-html-components or dash-core-components do not have to upgrade dash.
  2. The actual function signature of __init__ does not change, so auto complete tools which rely on the inspect module will work in Python 2 (so far, this seems to only be Jupyter Notebooks). In the current PR, the actual function signature is __init__(*args, **kwargs), and in Python3 we can use inspect.Signature introduced in Python 3.3 to change the arguments in the function signature. Most tools I have used (Atom, PyCharm) will just look at the module code so it would work for both.

and two drawback that I can think of:

  1. Some IDE's (Atom with the autocomplete-python package) actually display the default value (e.g n_clicks=DEFAULT, which might be misleading since we do not (currently) set the property to its default value (e.g. 0 for n_clicks), we set it to None.
  2. If we actually set default values, we would need to include the code to find and apply default properties in the __init__ function with special cases for properties where the default value is undefined. It would be nice if these were just taken from the arguments.

The current solution also may work better if the default arguments were included, for example if a property had undefined as a property (e.g. children), we could have in the function signature children='undefined' and it would behave as expected.

Anybody have thoughts on this or other drawbacks I haven't thought of, or have on opinion on which solution you prefer? I am personally for the current solution since I think the actual default value should be explicitly stated in the class code for documentation purposes (right now it is not in the docstring, the best place I know to look for it is the src/components/component.js).
@plotly/dash

@nicolaskruchten
Copy link
Contributor

I think it might be nice to have a family of default values like RequiredString or OptionalInt and so on, which would signal to the user two bits of info: required vs optional and expected type. If "required" then that means you'll get run-time errors if not provided, and if "optional" it means at run-time the underlying component will provide a default value. The type could be inferred from the component prop-types (just at the first level to begin with, nothing like OptionalDictOfString or anything).

It's definitely not Pythonic, but it would make for a nicer experience than DEFAULT IMO :)

@nicolaskruchten
Copy link
Contributor

(PS: the proposal above isn't just mine, it's something that was discussed briefly after the last call!)

@rmarren1
Copy link
Contributor Author

rmarren1 commented Jul 4, 2018

That seems better than DEFAULT, but I think we would then introduce a dependence of new component library versions on the new dash version, since we should probably introduce all of those new objects in dash/development rather than the archetype or component modules. Not a big deal, I have a check for this right now: https://github.com/rmarren1/dash-html-components/blob/8ba05ba8cc8a0abf245498d2e61b604a669fe480/dash_html_components/__init__.py#L18

I think we have all the functionality of this already, albeit without the default value type being written out as they would be if we included those class names as default arguments (but that information is written in the docstring, the actual default value is not included there currently). E.g., errors are thrown if required arguments are not included, and optional and not included values are not passed to the base Component constructor and therefore get the default value defined in the react code.

@rmarren1
Copy link
Contributor Author

Added more meaningful default arguments as discussed last standup, Component.UNDEFINED and Component.REQUIRED

@rmarren1 rmarren1 mentioned this pull request Jul 18, 2018
@chriddyp chriddyp added the dash-meta-sponsored Work items whose development has been sponsored by a commercial partner https://plot.ly/dash/pricing label Jul 24, 2018
else:
return (
'Table(' +
repr(getattr(self, self._prop_names[0], None)) + ')')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see room for improvement in making these classes even more minimal? I don't think we have to do it in this PR, but in the future it might be nice if these classes were able to inherit more of this logic, mainly because it looks pretty gnarly 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a lot of it could be moved into the base component (pretty much everything that is not an attribute assignment, and even some of those appear to be the same). I'll make a new issue for this

@chriddyp
Copy link
Member

Alright, I'm very happy with this. 💃 from me!

@rmarren1 rmarren1 merged commit 8a9820c into plotly:master Aug 1, 2018
Dash - A Pleasant and Productive Developer Experience automation moved this from In progress to Done Aug 1, 2018
@rmarren1 rmarren1 mentioned this pull request Aug 7, 2018
@Akronix
Copy link
Contributor

Akronix commented Oct 5, 2018

Awesome! any docs or hint on how to integrate this with our IDE or editor?

@rmarren1
Copy link
Contributor Author

rmarren1 commented Oct 5, 2018

What editor are you using? A lot of editors I tested work out the box if you install dash-html-components and dash-core-components globally.

@Akronix
Copy link
Contributor

Akronix commented Oct 11, 2018

I'm using geany.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dash-meta-sponsored Work items whose development has been sponsored by a commercial partner https://plot.ly/dash/pricing
Development

Successfully merging this pull request may close these issues.

None yet

4 participants