Skip to content

Replace ./R/sysdata.rda with ./R/data_gfont_info.R - #294

Merged
cpsievert merged 1 commit into
masterfrom
google_fonts_file
Mar 30, 2021
Merged

cpsievert merged 1 commit into
masterfrom
google_fonts_file

Conversation

@schloerke

Copy link
Copy Markdown
Collaborator

I can't see what changes in the sysdata.rda file and most times the file differences is because of OS, not content.

Since we are doing nothing fancy inside the structure, we can use dput() and source it on load.

Pull Request

Before you submit a pull request, please ensure you've completed the following checklist

  • Ensure there is an already open and relevant GitHub issue describing the problem in detail and you've already received some indication from the maintainers that they are welcome to a contribution to fix the problem. This helps us to prevent wasting anyone's time.

  • Add unit tests in the tests/testthat directory.

  • This project uses roxygen2 for documentation. If you've made changes to documentation, run devtools::document().

  • Run devtools::check() (or, equivalently, click on Build->Check Package in the RStudio IDE) to make sure your change did not add any messages, warnings, or errors.

    • Note there is a decent chance that some tests were already failing before your changes. Just make sure you haven't introduced any new ones.
  • Ensure your code changes follow the style outlined in http://r-pkgs.had.co.nz/style.html

  • Add an entry to NEWS.md concisely describing what you changed.

@schloerke
schloerke requested a review from cpsievert March 30, 2021 15:16
@schloerke schloerke changed the title no mo .rda file! Replace ./R/sysdata.rda with ./R/data_gfont_info.R Mar 30, 2021
@cpsievert
cpsievert merged commit be5d481 into master Mar 30, 2021
@cpsievert
cpsievert deleted the google_fonts_file branch March 30, 2021 15:29
@wch

wch commented Mar 30, 2021

Copy link
Copy Markdown
Collaborator

I'd suggest adding a check that the output matches the input, because dput is not perfect. For example, see:
https://github.com/rstudio/fontawesome/blob/0152548f07dd8d17ddf4940c6b3355b028d789a2/data-raw/fontawesome-update.R#L401-L405

Also, because of the way that dput chooses to wrap lines, any changes to the input data are likely to cause large diffs that are very difficult to inspect, so being able to see relevant changes will still be an issue even after this change. In fontawesome, we avoided this by writing to inline CSV content using textConnection(). I don't think the exact same thing could be done here, because of the list-columns, but it's something worth considering.

Here's the function that generates the code in FA. (Note that it's actually semicolon-delimited to avoid issues with escaping commas.)
https://github.com/rstudio/fontawesome/blob/0152548f07dd8d17ddf4940c6b3355b028d789a2/data-raw/fontawesome-update.R#L353-L406
And the resulting output:
https://github.com/rstudio/fontawesome/blob/master/R/fa_v4_v5.R

Incidentally, writing it out as text instead of .rda resulted in a source package that was 900kB instead of 1.2MB.

cpsievert added a commit that referenced this pull request Mar 30, 2021
cpsievert added a commit that referenced this pull request Mar 30, 2021
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