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

WIP Initial version of updated code for R transpiler #449

Closed
wants to merge 27 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e39bcf1
Initial version of updated code for R transpiler
Nov 5, 2018
23bb710
Fixed creation of R functions for components
Nov 5, 2018
9ce9509
Fixed creation of R functions for components, triple quote issue
Nov 5, 2018
ed5d5db
Edits related to PR comments
Nov 9, 2018
3df1cd6
Edited _all_keywords.py so R/Py kwlists similarly named.
Nov 9, 2018
451283f
Updated base_component.py kwlist >> python_keywords
Nov 9, 2018
5998545
Attempting to resolve pylint warnings
Nov 9, 2018
6606943
Resolve pylint warning by removing unused generate_class_r code
Nov 9, 2018
4575b9f
Updated base_component.py
Nov 10, 2018
9bb7603
Updated component_loader.py
Nov 10, 2018
f1e47f5
Merge branch 'master' into R
rpkyle Nov 10, 2018
f4cd844
Updated documentation for generate_classes_r
Nov 10, 2018
aae11d6
Updated base_component.py to support .Rd without roxygen2
Nov 11, 2018
62bf001
Updated component_loader.py to import generate_help_file_r
Nov 11, 2018
89b7b85
Updated component_loader.py and base_component.py
Nov 12, 2018
664e516
Modified base_component.py to address propNames resolution problem
Nov 12, 2018
3bf21b3
Updated keyword list to include R reserved words
Nov 23, 2018
84a6745
Changes to support transpiling of JSON>>R within Python
Nov 23, 2018
2a4ac5a
Changes to support transpiling of JSON>>R within Python
Nov 23, 2018
3341d8e
Restore export_string generation
Nov 23, 2018
0d7943d
Updates to component loader/base component
Nov 27, 2018
e182736
Merged RPK branch
Nov 27, 2018
13d0d71
Updates to generate_js_metadata_r
Nov 27, 2018
8a43d98
Removed external_url parsing
Nov 28, 2018
8d2d538
Clarified comments in code
Nov 28, 2018
008598f
Added statements to copy JS deps into inst/lib
Nov 28, 2018
918a073
Updated base_component.py
Nov 28, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 232 additions & 0 deletions dash/development/base_component.py
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,120 @@ def __repr__(self):
required_args = required_props(props)
return c.format(**locals())

def generate_class_string_r(typename, props, description, namespace):
"""
Dynamically generate class strings to have nicely formatted docstrings,
keyword arguments, and repr
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can update this docstring to reflect that it generate R components. docstring, keywords arguments, repr are python things that may not relate to R.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this makes sense. When I first started editing, I think I was trying too hard to maintain parity between the existing code and the R-related bits I introduced. I'll revise so that my additions reflect R conventions when appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.S. Thanks for having a look!


Inspired by http://jameso.be/2013/08/06/namedtuple.html

Parameters
----------
typename
props
description
namespace

Returns
-------
string

"""
# TODO _prop_names, _type, _namespace, available_events,
# and available_properties
# can be modified by a Dash JS developer via setattr
# TODO - Tab out the repr for the repr of these components to make it
# look more like a hierarchical tree
# TODO - Include "description" "defaultValue" in the repr and docstring
#
# TODO - Handle "required"
#
# TODO - How to handle user-given `null` values? I want to include
# an expanded docstring like Dropdown(value=None, id=None)
# but by templating in those None values, I have no way of knowing
# whether a property is None because the user explicitly wanted
# it to be `null` or whether that was just the default value.
# The solution might be to deal with default values better although
# not all component authors will supply those.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove those comments, they contains todo's that are specifics to python classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. I'll also finish commenting my additions where appropriate.

c = '''{docstring}

html{typename} <- function(..., {default_argtext}) {{

component <- list(
props = list(
{default_paramtext}
),
type = '{typename}',
namespace = '{namespace}',
propNames = c({list_of_valid_keys}),
package = '{package_name}'
)

component$props <- filter_null(component$props)
component <- append_wildcard_props(component, wildcards = c({default_wildcards}), ...)

structure(component, class = c('dash_component', 'list'))
}}
'''

filtered_props = reorder_props(filter_props(props))
# pylint: disable=unused-variable
list_of_valid_wildcard_attr_prefixes = repr(parse_wildcards(props))
# pylint: disable=unused-variable
list_of_valid_keys = repr(list(map(str, filtered_props.keys())))
list_of_valid_keys = list_of_valid_keys[1:-1]
package_name = make_package_name(namespace)
# pylint: disable=unused-variable
docstring = create_docstring_r(
component_name=typename,
props=filtered_props,
events=parse_events(props),
description=description).replace('\r\n', '\n')
Copy link
Contributor

Choose a reason for hiding this comment

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

I added that .replace('\r\n', '\n') for generating the docstring on windows because git doesn't auto replace them when they are in docstring. Not sure this problem is the same with R.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it is a consideration in R as well, though I think I should remove the \n and replace with a single space, since R's help viewer does its own line wrapping without adding linefeeds.


# pylint: disable=unused-variable
Copy link
Contributor

Choose a reason for hiding this comment

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

# want to remove data-*, aria-*
events = '[' + ', '.join(parse_events(props)) + ']'
prop_keys = list(props.keys())
default_paramtext = ''
default_wildcards = ''
wildcard_list = ''
for key in props:
if '*' in key:
wildcard_list.join(key)
if 'children' in props:
prop_keys.remove('children')
default_argtext = "children=NULL, "
# pylint: disable=unused-variable
argtext = 'children=children, **args'
else:
default_argtext = ""
argtext = '**args'
# in R, we set parameters with no defaults to NULL, here we'll do that if no default value exists
default_wildcards += ", ".join(
[('\'{:s}\''.format(p))
for p in prop_keys
if '*' in p]
)
default_argtext += ", ".join(
[('{:s}={}'.format(p, props[p]['defaultValue']['value'])
if 'defaultValue' in props[p] else
'{:s}=NULL'.format(p))
for p in prop_keys
if not p.endswith("-*") and
p not in keyword.kwlist and
Copy link
Contributor

Choose a reason for hiding this comment

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

⚡ This keyword list filter is for python keywords, won't be the same in R.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually now we manually maintain a file of python keywords (https://github.com/plotly/dash/blob/master/dash/development/_all_keywords.py). Perhaps we can re-name that to python_keywords and make a new one called R_keywords.

That is, if this problem even exists in R. Can you define a function with argument names that are the same as a reserved keyword in R?

Copy link
Contributor Author

@rpkyle rpkyle Nov 8, 2018

Choose a reason for hiding this comment

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

That is, if this problem even exists in R. Can you define a function with argument names that are the same as a reserved keyword in R?

If the function argument matches a reserved word (which is not first escaped with backticks), the R parser will throw an error:

Error: unexpected 'if' in "myfun <- function(if"

The help page (also produced when you type ?reserved) is here:

https://stat.ethz.ch/R-manual/R-devel/library/base/html/Reserved.html

I've gone ahead and produced a GitHub gist including reserved words in R, similar to the keyword list already available for Python. I can submit it as a pull request if it would be helpful.

https://gist.github.com/rpkyle/622155e24f603fc54a664b06f8036f3d

Copy link
Contributor

Choose a reason for hiding this comment

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

You can include those keywords in https://github.com/plotly/dash/blob/master/dash/development/_all_keywords.py as a new variable r_keywords and use it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, will do.

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'm in the process of updating the code I've written, so please hold off on reviewing the updates to this PR for a day or so. Nicolas suggested remerging my new branch with R, so that we can keep the current PR alive.

Copy link
Contributor Author

@rpkyle rpkyle Nov 28, 2018

Choose a reason for hiding this comment

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

OK, I think I've made the minimal sufficient set of changes required to base_component.py necessary to load dashR with the current version of dash-html-components and dash-core-components. Mind having another look at the changes?

@rmarren1
@T4rk1n

p not in ['setProps']]
)
default_paramtext += ", ".join(
[('{:s}={:s}'.format(p, p)
if p != "children" else
'{:s}=c(children, assert_valid_children(..., wildcards = c({:s})))'.format(p, default_wildcards))
for p in props.keys()
if not p.endswith("-*") and
p not in keyword.kwlist and
Copy link
Contributor

Choose a reason for hiding this comment

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

p not in ['setProps']]
)
required_args = required_props(props)
return c.format(**locals())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like them to be explicitly stated instead of locals(), then we can remove all those # pylint: disable=unused-variable and easily see what variables are not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I think we talked about this when you explained list comprehensions to me a while ago. Will remove the shorthand and explicitly declare the variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this, and we should do it in the Python version as well. I've added a note here: #311


# pylint: disable=unused-argument
def generate_class_file(typename, props, description, namespace):
Expand Down Expand Up @@ -421,6 +535,36 @@ def generate_class_file(typename, props, description, namespace):
f.write(import_string)
f.write(class_string)

# pylint: disable=unused-argument
def generate_class_file_r(typename, props, description, namespace):
"""
Generate a R class file (.R) given a class string

Parameters
----------
typename
props
description
namespace

Returns
-------

"""
import_string =\
"# AUTO GENERATED FILE - DO NOT EDIT\n\n"
class_string = generate_class_string_r(
typename,
props,
description,
namespace
)
file_name = "{:s}.R".format(typename)

file_path = os.path.join(namespace, file_name)
with open(file_path, 'w') as f:
f.write(import_string)
f.write(class_string)

# pylint: disable=unused-argument
def generate_class(typename, props, description, namespace):
Expand All @@ -445,6 +589,27 @@ def generate_class(typename, props, description, namespace):
result = scope[typename]
return result

def generate_class_r(typename, props, description, namespace):
Copy link
Contributor

Choose a reason for hiding this comment

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

⚡ the generate_class for python is for dynamically loaded components, we don't do that in R.

"""
Generate a python class object given a class string

Parameters
----------
typename
props
description
namespace

Returns
-------

"""
string = generate_class_string_r(typename, props, description, namespace)
scope = {'Component': Component, '_explicitize_args': _explicitize_args}
# pylint: disable=exec-used
exec(string, scope)
result = scope[typename]
return result

def required_props(props):
"""
Expand Down Expand Up @@ -507,6 +672,49 @@ def create_docstring(component_name, props, events, description):
for p, prop in list(filter_props(props).items())),
events=', '.join(events))

def create_docstring_r(component_name, props, events, description):
"""
Create the Dash component docstring

Parameters
----------
component_name: str
Component name
props: dict
Dictionary with {propName: propMetadata} structure
events: list
List of Dash events
description: str
Component description

Returns
-------
str
Dash component docstring
"""
# Ensure props are ordered with children first
props = reorder_props(props=props)

desctext = ''
desctext += "\n".join(
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to append, just do desctext = "\n".join(

[('#\' @param {:s} {:s}'.format(p, props[p]['description'].replace('\n', ' ')))
for p in props.keys()
if not p.endswith("-*") and
p not in keyword.kwlist and
Copy link
Contributor

Choose a reason for hiding this comment

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

p not in ['setProps']]
)

return('''#' {name} component
#' @description See <https://developer.mozilla.org/en-US/docs/Web/HTML/Element/{name}>
#' @export
#' @param ... The children of this component and/or 'wildcards' of the form: `data-*` or `aria-*`
{desctext}
'''
).format(
name=component_name,
description=description,
desctext=desctext,
events=', '.join(events))

def parse_events(props):
"""
Expand Down Expand Up @@ -550,6 +758,25 @@ def parse_wildcards(props):
list_of_valid_wildcard_attr_prefixes.append(wildcard_attr[:-1])
return list_of_valid_wildcard_attr_prefixes

def parse_wildcards_r(props):
Copy link
Contributor

Choose a reason for hiding this comment

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

🐫 I do not think this function is necessary, it is the same as parse_wildcards currently. We can keep this as one function used by the R and Python generation until we have a reason to re-write it.

"""
Pull out the wildcard attributes from the Component props

Parameters
----------
props: dict
Dictionary with {propName: propMetadata} structure

Returns
-------
list
List of Dash valid wildcard prefixes
"""
list_of_valid_wildcard_attr_prefixes = []
for wildcard_attr in ["data-*", "aria-*"]:
if wildcard_attr in props.keys():
list_of_valid_wildcard_attr_prefixes.append(wildcard_attr[:-1])
return list_of_valid_wildcard_attr_prefixes

def reorder_props(props):
"""
Expand Down Expand Up @@ -837,3 +1064,8 @@ def js_to_py_type(type_object, is_flow_type=False, indent_num=0):
# All other types
return js_to_py_types[js_type_name]()
return ''

def make_package_name(namestring):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be make_package_name_r to be more explicit?

# first, *rest = namestring.split('_') Python 3
Copy link
Contributor

Choose a reason for hiding this comment

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

first, rest = namestring.split('_')[0], namestring.split('_')[1:]
return first + ''.join(word.capitalize() for word in rest)
12 changes: 11 additions & 1 deletion dash/development/component_loader.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import collections
import json
import os
#from dash.development.base_component import generate_class
Copy link
Contributor

Choose a reason for hiding this comment

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

from .base_component import generate_class
from .base_component import generate_class_file

from .base_component import generate_class_r
from .base_component import generate_class_file_r
#from dash.development.base_component import generate_class_file
Copy link
Contributor

Choose a reason for hiding this comment

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


def _get_metadata(metadata_path):
# Start processing
Expand Down Expand Up @@ -94,8 +97,15 @@ def generate_classes(namespace, metadata_path='lib/metadata.json'):
componentData['description'],
namespace
)
generate_class_file_r(
name,
componentData['props'],
componentData['description'],
namespace
)

# Add an import statement for this component
# RK: need to add import statements in R namespace file also
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that still todo ?

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've added export() statements to the NAMESPACE file for the R package, don't think we'll need to provide an import statement. Will remove the comment.

with open(imports_path, 'a') as f:
f.write('from .{0:s} import {0:s}\n'.format(name))

Expand Down