shinyobjects icon indicating copy to clipboard operation
shinyobjects copied to clipboard

Don't formally import styler, but use `::`

Open lorenzwalthert opened this issue 10 months ago • 4 comments

I am the author of the R package styler, which you use in your code. I repeatedly got strange and hard-to-reproduce reverse dependency errors when submitting a new version of my package to CRAN: Running R CMD CHECK on your package with a new version of {styler} results in problems like https://github.com/HenrikBengtsson/R.cache/issues/51 that I believe could be easily avoided by not importing styler in shiny objects. So instead of importing style_text() into your NAMESPACE and use style_file() in your source code, you could use styler::style_file() and not import style_text() into your NAMESPACE. I don't believe it would affect the user experience of your package.

Would you be open to that change? It has the potential to save me hours of debugging and stress.

lorenzwalthert avatar Apr 07 '24 15:04 lorenzwalthert

Yikes, sorry for the headache. I'll get on it this week.

rjake avatar Apr 08 '24 12:04 rjake

Hi @rjake, thanks for getting back to me so quickly. In the mean time, my styler submission made it to CRAN, so no immediate stress or debugging efforts required anymore from my side. I am not sure the fix solved the issue, so I'd be grateful if you would still consider the proposed change, but there is no need for it to happen this week. Also, you did nothing wrong 😀

lorenzwalthert avatar Apr 08 '24 12:04 lorenzwalthert

I'm happy to make a change but I am not fully following the suggestion. I am using style_text() only and it is in the code like this:

image

Is your suggestion:

  • to remove the line @importFrom styler style_text?
  • to replace styler::style_text() with styler::style_file?
  • both?

I use roxygen2 to edit the NAMESPACE file rather than writing it by hand. Please let me know which interpretation is correct and I'll make the change. If I need to replace the function, I may reach out with more questions.

Also, thanks for making such a great package! I use it every day and have it mapped as a keyboard shortcut.

rjake avatar Apr 10 '24 02:04 rjake

If you use styler::style_text(), you don’t need @importFrom styler style_text. The latter you only need if you use style_text(). So just drop that roxygen comment.

lorenzwalthert avatar Apr 10 '24 07:04 lorenzwalthert