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

Add documentation and minor refactoring to allow PDFR to pass devtools::check() without errors #4

Open
elipousson opened this issue Mar 21, 2023 · 2 comments

Comments

@elipousson
Copy link
Contributor

Hello @AllanCameron! I really appreciate you creating this package – I'm using it to extract text from some older reports at work and reformat the text into a tabular structure. When I went to look-up the documentation for pdfpage() I realized that exporting the documentation is one of the things that remained incomplete with the package.

I just forked the package and went ahead and made the following changes to get the documentation filled in and get the package to pass devtools::check() without errors. Here are all the changes I made:

  • Remove existing NAMESPACE file to replace with NAMESPACE generated by roxygen2
  • Add httr and grDevices to Imports and move Rcpp11 to Suggests
  • Add package level documentation to handle imports from ggplot2, Rcpp, and httr
  • Add package level metadata with codemetar::write_codemeta()
  • Update function documentation to import from grid and grDevices
  • Re-formated the DESCRIPTION with use_tidy_description() (and added URLs + Authors)
  • Update license with use_mit_license()
  • Added a utils.R file to add a call utils::globalVariables()
  • Replace the markdown README with a Rmd file
  • Exported the testfiles data with use_data()
  • Disabled execution of a broken test in test-pdrf.R
  • Disabled execution of a broken example for draw_glyph()

If this all looks good, I'm happy to open a pull request.

I also noticed that the size parameter in pdfgraphics() may need to be changed to linewidth to work with the most recent version of ggplot2 (see this post for more information). I can test this out and add it to the same pull request or open a separate issue if you'd like to discuss.

@elipousson
Copy link
Contributor Author

The package is still kicking back a note and two warnings:

  • Note: "Found if() conditions comparing class() to string"
  • Significant warning: "utilities.cpp:186:26: warning: bitwise or with non-zero value always evaluates to true [-Wtautological-bitwise-compare]"
  • Additional warning: "Non-staged installation was used"

I started addressing the note by replacing class(pdf) == "character" with rlang::is_character() with the assumption that an additional dependency for rlang should be fine if you are already importing ggplot2. I'd also be glad to convert the stop() errors into abort() or (if you're OK with adding a dependency on cli) cli::cli_abort(). Again, happy to handle these in a separate pull request or avoid adding these additional dependencies altogether if you prefer.

@AllanCameron
Copy link
Owner

Hi Eli

Thanks very much for this. I had rather given up developing this package as you can probably tell. I will have to re familiarise myself with the code base. Please do open a pull request; I'm very happy to have your input

Allan

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

No branches or pull requests

2 participants