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

Dash for R v0.3.0 #175

Merged
merged 45 commits into from
Feb 13, 2020
Merged

Dash for R v0.3.0 #175

merged 45 commits into from
Feb 13, 2020

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Feb 12, 2020

This PR implements several new features related to the Dash for R v0.3.0 release:

[0.3.0] - 2020-02-12

Added

  • Support for config-aware relative paths #172
  • Support index customization and index templates #168
  • Application titles may be set using the app$title() method, for parity with Dash for Python's app.title syntax #168

Changed

  • Dash for R now requires dashCoreComponents v1.8.0
  • Dash for R now requires dashTable v4.6.0
  • Automatically set routes and requests pathname prefixes if DASH_APP_NAME environment variable has been set #165

Deprecated

  • Application titles can no longer be set using name parameter, which is now deprecated with a warning, for parity with Dash for Python #168
  • Removed DASH_HOST and DASH_PORT, Dash for R now respects HOST and PORT #167

@alexcjohnson

rpkyle and others added 30 commits August 23, 2019 09:23
* rename pruned_errors to prune_errors

* rename pruned to prune

* provide support for no_update in Dash for R (#111)
* rename pruned_errors to prune_errors

* rename pruned to prune

* 🔨 handle stop errors

* 🚨 add test for stop errors
* Add line number context to stack traces when srcrefs are available (#133)

* ✨ Support line #s when in debug mode

* ✨ Add use_viewer option for RStudio

* 🚨 Add soft and hard hot reloading tests
* ✨ initial support for meta tags

* support arbitrary tags

* 🚨 add tests

* 🔬 add asserts

* add reference to meta tag PR

* ⏩ indent meta tags
* add eager_loading parameter

* 📛 add buildFingerprint

* 📛 add checkFingerprint

* use getDependencyPath, + 🐾/Etag support

* updates to support async

* ✨ properly support gz compression

* 🐛 post-async fixes for CSS handling
…races (#137)

* 🚚 upgrade dash-renderer to v1.2.2, 🔨 fix stack traces

* 🚚 add polyfill.js
* refactor resolvePrefix

* camel case resolve_prefix

* Update R/utils.R

Co-authored-by: HammadTheOne <30986043+HammadTheOne@users.noreply.github.com>
HammadTheOne and others added 5 commits February 11, 2020 19:26
@rpkyle rpkyle added high priority parity Modifications to improve parity across Dash implementations labels Feb 12, 2020
@rpkyle rpkyle self-assigned this Feb 12, 2020
- Application titles may be set using the `app$title()` method, for parity with Dash for Python's `app.title` syntax [#168](https://github.com/plotly/dashR/pull/168)

### Changed
- Dash for R now requires `dashCoreComponents` v1.8.0
Copy link
Contributor

Choose a reason for hiding this comment

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

The table version bump should also be mentioned.

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 c03c6c1

CHANGELOG.md Outdated

### Changed
- Dash for R now requires `dashCoreComponents` v1.8.0
- Rename `DASH_HOST` to `HOST` and `DASH_PORT` to `PORT` [#167](https://github.com/plotly/dashR/pull/167)
Copy link
Contributor

Choose a reason for hiding this comment

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

This goes beyond the scope of ### Changed and could be ### Deprecated (or removed)+### Added entries instead. Same for DASH_APP_NAME below. As this is a 0.x release it's not technically 100% necessary. At your discretion.

Copy link
Contributor Author

@rpkyle rpkyle Feb 12, 2020

Choose a reason for hiding this comment

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

fixed in 030adb4 and 744b696

R/dash.R Outdated Show resolved Hide resolved
R/dash.R Outdated Show resolved Hide resolved
@@ -1266,6 +1400,24 @@ Dash <- R6::R6Class(

# akin to https://github.com/plotly/dash/blob/d2ebc837/dash/dash.py#L338
# note discussion here https://github.com/plotly/dash/blob/d2ebc837/dash/dash.py#L279-L284
custom_index = NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo at line 965: dire ctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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, fixed, fixed in 49c97da

@@ -457,22 +457,25 @@ valid_seq <- function(params) {
}
}

resolve_prefix <- function(prefix, environment_var, base_pathname) {
resolvePrefix <- function(prefix, environment_var, base_pathname) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering why this function is changed to camelCase when many other functions are snake_case.

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 going to open a PR to migrate all functions to camelCase and variables to snake_case, which follows the generally accepted style rules for R. That will likely occur within the next week, prior to CRAN submission.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cf. https://google.github.io/styleguide/Rguide.html, though they use Pascal case instead of camel case, and maybe we should also. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Kk, thx. Pick one, enforce it. Move on :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:neckbeard: 📘 🤺

rpkyle and others added 3 commits February 12, 2020 14:36
Co-Authored-By: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>
<html>
<head>
{%meta_tags%}
<title>Testing Again</title>
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Feb 12, 2020

Choose a reason for hiding this comment

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

What happens if you set both app$title and the template? The template wins? Maybe adding some default title app$title("On no! Not again!") in this test makes that behavior more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you set app$title, it wins. I suspect this is what we want, rather than the template taking precedence, though I cannot say for sure.

@rpkyle rpkyle merged commit f6a2172 into master Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority parity Modifications to improve parity across Dash implementations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants