Uh oh!
There was an error while loading. Please reload this page.
Conversation
T4rk1n
left a comment
There was a problem hiding this comment.
Look good so far, couple things to remove as I don't think they translate from python to R.
Also, I'd like those methods (and the python generation too) to be in a separate file as it should be the file for the python component class and associated methods. But that can be in another PR.
| def generate_class_string_r(typename, props, description, namespace): | ||
| """ | ||
| Dynamically generate class strings to have nicely formatted docstrings, | ||
| keyword arguments, and repr |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
P.S. Thanks for having a look!
| # 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. |
There was a problem hiding this comment.
Remove those comments, they contains todo's that are specifics to python classes.
There was a problem hiding this comment.
Will do. I'll also finish commenting my additions where appropriate.
| component_name=typename, | ||
| props=filtered_props, | ||
| events=parse_events(props), | ||
| description=description).replace('\r\n', '\n') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| events=parse_events(props), | ||
| description=description).replace('\r\n', '\n') | ||
| # pylint: disable=unused-variable |
| '{:s}=NULL'.format(p)) | ||
| for p in prop_keys | ||
| if not p.endswith("-*") and | ||
| p not in keyword.kwlist and |
There was a problem hiding this comment.
⚡ This keyword list filter is for python keywords, won't be the same in R.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| result = scope[typename] | ||
| return result | ||
| def generate_class_r(typename, props, description, namespace): |
There was a problem hiding this comment.
⚡ the generate_class for python is for dynamically loaded components, we don't do that in R.
| props = reorder_props(props=props) | ||
| desctext = '' | ||
| desctext += "\n".join( |
There was a problem hiding this comment.
No need to append, just do desctext = "\n".join(
| import collections | ||
| import json | ||
| import os | ||
| #from dash.development.base_component import generate_class |
| from .base_component import generate_class_r | ||
| from .base_component import generate_class_file_r | ||
| #from dash.development.base_component import generate_class_file |
| ) | ||
| # Add an import statement for this component | ||
| # RK: need to add import statements in R namespace file also |
There was a problem hiding this comment.
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.
| [('#\' @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 |
| list_of_valid_wildcard_attr_prefixes.append(wildcard_attr[:-1]) | ||
| return list_of_valid_wildcard_attr_prefixes | ||
| def parse_wildcards_r(props): |
There was a problem hiding this comment.
🐫 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.
| return js_to_py_types[js_type_name]() | ||
| return '' | ||
| def make_package_name(namestring): |
There was a problem hiding this comment.
Can it be make_package_name_r to be more explicit?
| return '' | ||
| def make_package_name(namestring): | ||
| # first, *rest = namestring.split('_') Python 3 |
| # >>> import keyword | ||
| # >>> keyword.kwlist | ||
| kwlist = set([ |
There was a problem hiding this comment.
We should change this to python_keywords then to be consistent. I think we should do it here, it's only 4 lines and I don't think it needs its own PR.
| required_args = required_props(props) | ||
| return c.format(**locals()) | ||
| return c.format(typename=typename, |
rpkyle
commented
Dec 6, 2018
Closing this PR in favour of #483. |
No description provided.