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

Assorted fixes required for CRAN submission #1186

Merged
merged 21 commits into from
May 4, 2020
Merged

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Apr 9, 2020

This PR proposes several enhancements and minor fixes in the R package generator, based on requests from CRAN maintainers:

Specifically, two new YAML keys are supported in dash-info.yaml:

  • pkg_copyright: when present, will add in a Copyright: line including the string value (see https://cran.r-project.org/web/packages/policies.html)
  • pkg_authors: when present, dash-generate-components will use this string as-is for Authors@R:, which CRAN prefers to Author: and Maintainer: elements within DESCRIPTION. When this string is not present, the generator will create it based on the author key within package.json, and assume that the author is the maintainer.

@rpkyle rpkyle self-assigned this Apr 9, 2020
@rpkyle rpkyle marked this pull request as ready for review April 9, 2020 19:41
@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Apr 23, 2020

@rpkyle I think we should drop _asset_dist and just use _js_dist for now. While the _asset_dist is an interesting addition, I think we should rework css/js/asset in depth and with all Dash flavors in mind before committing to supporting additional props. _js_dist is not perfect but it covers all usages with _css_dist

This PR Portions of this PR would be superseded by #1078.

@rpkyle rpkyle changed the title WC handling fixes & arbitrary extension support in R package generator WC handling and line wrapping fixes Apr 23, 2020
@rpkyle rpkyle changed the base branch from dev to 481-arbitrary-extensions April 23, 2020 23:37
@rpkyle rpkyle changed the title WC handling and line wrapping fixes Assorted fixes required for CRAN submission Apr 30, 2020
@plotly plotly deleted a comment from chriddyp Apr 30, 2020
@rpkyle
Copy link
Contributor Author

rpkyle commented May 1, 2020

@alexcjohnson @Marc-Andre-Rivet This is ready for your review, and should resolve the CRAN maintainer concerns. I'd like to merge this today if possible, so Marc-André can use it for package builds starting with the upcoming Dash release. 🙏

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💄 There are a bunch of places here the indentation is just changed from 4 to 8 spaces inside parens, either function args or if conditions - please put back the original 4 spaces. Also there's a ._r_components_generation.py.swp file committed -> should be .gitignore'd?

Aside from that these changes look good to me!

@rpkyle
Copy link
Contributor Author

rpkyle commented May 2, 2020

💄 There are a bunch of places here the indentation is just changed from 4 to 8 spaces inside parens, either function args or if conditions - please put back the original 4 spaces. Also there's a ._r_components_generation.py.swp file committed -> should be .gitignore'd?

Aside from that these changes look good to me!

Sure, not entirely understanding what happened there, but the linter demanded more spaces, so I added them. 🤷‍♂️ I can try removing them, but if it fails the check I'll have to determine what led to the (new) warnings.

Will add the vim-generated swapfile, should probably .gitignore all .swp files, never any reason to commit them.

@rpkyle
Copy link
Contributor Author

rpkyle commented May 2, 2020

Also there's a ._r_components_generation.py.swp file committed -> should be .gitignore'd?

fixed in ae28bac and bc903bc

@rpkyle
Copy link
Contributor Author

rpkyle commented May 3, 2020

💄 There are a bunch of places here the indentation is just changed from 4 to 8 spaces inside parens, either function args or if conditions - please put back the original 4 spaces.

OK, restored in 06dbc8d. Will try to sort out why that happened, might be a difference in configuration between pylint on my machine vs. CircleCI.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

This PR should be retargeted to dev now that #1078 is merged, yes? Anyway after that and the Windows build is sorted out I think it's ready to go! 💃

@rpkyle
Copy link
Contributor Author

rpkyle commented May 4, 2020

This PR should be retargeted to dev now that #1078 is merged, yes? Anyway after that and the Windows build is sorted out I think it's ready to go! 💃

@alexcjohnson I've taken your advice (thanks! 👏 ) on removing the spaces within the --r-suggests statement inside of package.json in plotly/dash-core-components, and the workaround enables the test to pass, but we still haven't sorted out why it fails (and we should).

I've opened https://github.com/plotly/dash-core/issues/184 in the meantime, and 019df65 will add back the spaces we have now omitted when necessary.

@rpkyle rpkyle changed the base branch from 481-arbitrary-extensions to dev May 4, 2020 15:04
@rpkyle rpkyle merged commit fbb38c4 into dev May 4, 2020
@rpkyle rpkyle deleted the 757-generator-enhancements branch May 4, 2020 15:16
rpkyle added a commit that referenced this pull request May 9, 2020
* wrap descriptions at 60 chars

* fix wildcard handling

* support for non-CSS/JS deps in assets

* add value tag for .Rd files

* add copyright field to DESCRIPTION

* add Authors@R line to DESCRIPTION

* fix --r-suggests lstrip bug

* +sp after , if missing in R imp/sugg/deps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants