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

Provide string interpolation support for async/dynamic dep attributes in R #1048

Merged
merged 25 commits into from
Jan 7, 2020

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Dec 13, 2019

This PR proposes to add support for generating async or dynamic attributes within the JavaScript metadata Dash for R uses to manage dependencies.

Additionally, improvements to the package generator have been introduced:

  • R package descriptions and titles are now populated using dash-info.yaml first, and then from information in package.json when it is unavailable or the relevant keys are missing
  • R package dependency versions (.js, .map, .css) are now properly generated to match versioning within the library of origin
  • if a vignettes directory is present, the generator will add a VignetteBuilder entry to DESCRIPTION and append knitr, rmarkdown to the Suggests field, but it will not build the vignettes itself (that requires running R CMD build and is out of scope at this time)
  • any examples that are included in docstrings and prepended with **Example Usage** will now be filtered out of the R help pages
  • KeepSource is set to true in generated R packages, to facilitate line numbering in stack traces
  • % within docstrings will silently and automatically be escaped when generating .Rd files within R packages, as these are comment characters in R help files and will break package installation
  • fatal errors during R package assembly (the absence of properly formatted author/maintainer information) will now terminate package generation with a diagnostic message

Closes #1002. Fixes plotly/dash-core#69, resolves plotly/dash-core#71.

@rpkyle rpkyle added this to the Dash v1.8 milestone Dec 13, 2019
@rpkyle rpkyle self-assigned this Dec 13, 2019
)
]
function_frame_body = ",\n".join(function_frame)
elif len(alldist) == 1:
rpp = alldist[0]["relative_package_path"]
dep = alldist[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not part of this change, but I'm having trouble understanding why we process an array of length one differently from an array of length N

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code doesn't really make much sense as written, but that's because I had trouble getting it to work with enumerate when I wrote it a year ago. I had only been programming in Python for a week or two, so this code is pretty much a giant pile of 💩. It needs some love and refactoring, no disagreement there.

# then return the properly formatted string if so, i.e.
# " async = TRUE,". a dependency can have async or
# dynamic elements, neither of these, but never both.
def check_async_type(dep):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would suggest renaming to get_** instead as this doesn't check but rather returns some specific value to use as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 3ff7607

@@ -0,0 +1,14115 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like 🔪 - there's no corresponding package.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 1d5a112

"R package.",
file=sys.stderr,
)
sys.exit(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed on slack: javascript docstrings should NOT contain escapes for % characters, that would look weird in JS (and in Python). So the right way to handle this is to automatically insert the escapes when translating from JS 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.

fixed in 52a3394

@Marc-Andre-Rivet
Copy link
Contributor

The tests failures we are seeing here are caused by a regression from the dcc.Graph responsiveness -- not related to this PR (https://circleci.com/gh/plotly/dash/13778#tests/containers/1)

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

Failures are coming from a regression in DCC. This looks fine. 💃

@rpkyle
Copy link
Contributor Author

rpkyle commented Jan 7, 2020

I'm going to add one last feature here -- to scrub examples from docstrings in packages such as DDK when generating the R packages, will relocate them to dash-info.yaml (at least for R, can leave them as-is for Python users).

@rpkyle
Copy link
Contributor Author

rpkyle commented Jan 7, 2020

I'm going to add one last feature here -- to scrub examples from docstrings in packages such as DDK when generating the R packages, will relocate them to dash-info.yaml (at least for R, can leave them as-is for Python users).

fixed in b471d35

@rpkyle rpkyle merged commit ee35164 into dev Jan 7, 2020
@rpkyle rpkyle deleted the 1042-async-rsupport branch January 7, 2020 19:41
chriddyp pushed a commit that referenced this pull request Jan 8, 2020
* ✨ async/dynamic support in R pkg deps

* 🔪 remove Authors block

* 🔪 insert package version number

* 🔨 use verbose 📦 title and desc from YAML

* ✨ autodetect vignettes

* check for author/maintainer address

* ✋ halt processing if fatal errors found

* autopopulate KeepSource

* auto-escape % in docstrings

* 🔪 filter examples from docstrings in R
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance something is slow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need async metadata support when generating R versions of component libraries
3 participants