Skip to content

Conversation

@rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Jan 31, 2020

This PR will add a private field to track the DASH_APP_NAME when set via the environment.

Closes plotly/streambed#14081.

@alexcjohnson @Marc-Andre-Rivet

@rpkyle rpkyle added enhancement parity Modifications to improve parity across Dash implementations labels Jan 31, 2020
@rpkyle rpkyle added this to the Dash v1.9 milestone Jan 31, 2020
@rpkyle rpkyle requested a review from HammadTheOne January 31, 2020 16:01
@rpkyle rpkyle self-assigned this Jan 31, 2020
@Marc-Andre-Rivet Marc-Andre-Rivet modified the milestones: Dash v1.9, Dash v1.10 Feb 4, 2020
DESCRIPTION Outdated
Imports:
dashHtmlComponents (== 1.0.2),
dashCoreComponents (== 1.6.0),
dashCoreComponents (== 1.7.0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dashCoreComponents (== 1.7.0),
dashCoreComponents (>= 1.7.0),

Probably best to change this to greater than, as 1.8.0 of dash-core-components has been released to master.

R/dash.R Outdated
assertthat::assert_that(is.logical(suppress_callback_exceptions))

# save relevant args as private fields
if (is.null(name) && Sys.getenv("DASH_APP_NAME") != "") {
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

@HammadTheOne HammadTheOne left a comment

Choose a reason for hiding this comment

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

Hi Ryan,

I checked out the code and it looks good; the implementation follows the Python version as much as it makes sense to, and the logic is sound. I tested it out locally and with a deployed app on Dash Enterprise, and it was able to locate the DASH_APP_NAME env. variable and assign that as the name.

Only change I made was allowing newer versions of dashCoreComponents, or installation of the package fails unless a user downgrades to the exact version specified in the description. We may even want to change this for the other dependencies, it would make it easier to transition from working locally with different versions of dash and component packages.

💃

@rpkyle rpkyle changed the title Add check for DASH_APP_NAME in case Dash app name parameter is NULL Use DASH_APP_NAME when available for prefixing Feb 6, 2020
@rpkyle rpkyle changed the title Use DASH_APP_NAME when available for prefixing Add private field to track DASH_APP_NAME Feb 6, 2020
@rpkyle
Copy link
Contributor Author

rpkyle commented Feb 6, 2020

The relevant changes were applied in #165, thus resolving https://github.com/plotly/streambed/issues/14081. Closing this one.

@rpkyle rpkyle closed this Feb 6, 2020
@rpkyle rpkyle deleted the 14081-check-app-name branch May 4, 2020 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement parity Modifications to improve parity across Dash implementations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants