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

Export of non-exported functions to enable other style guides to pass CRAN checks #974

Closed
3 tasks
Robinlovelace opened this issue Aug 8, 2022 · 7 comments · Fixed by #1043
Closed
3 tasks

Comments

@Robinlovelace
Copy link

Robinlovelace commented Aug 8, 2022

Please consider exporting the following functions:

  • is_function_call()
  • previous_non_comment()
  • scope_normalize()

Context: I'm working on {styler.equals} based on @lorenzwalthert's very handy template: https://github.com/lorenzwalthert/styler.yours

As mentioned in #860 tests do not pass due to these lines:

if (styler:::is_function_call(pd)) {

https://github.com/lorenzwalthert/styler.yours/blob/07b6689532aab92b1c60f106f9daf0cbb1fed6e3/R/rules-line-break.R#L14

https://github.com/lorenzwalthert/styler.yours/blob/ea4eb50e79886c5bf58dec51dff76aafbcdd4249/R/core.R#L21

@MichaelChirico advocated in #340 (comment) to export those functions to make it easier to for people building alternative styles building on this great package to release them to the masses.

@ShixiangWang
Copy link

Thanks @Robinlovelace

@Robinlovelace
Copy link
Author

Thank you for instigating this Shixiang, hopefully many people will benefit 🚀

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Aug 9, 2022

I am a bit hesitant to export these functions. First of all, they don't provide value for 99% of the users of {styler} and hence convolute the namespace, add aditional constraint on the development of {styler} (compatibility with downstream users). Another option is that you define these functions yourself (just c/p from styler) since they are relatively easy. Also, you needed a new {styler} release whenever you wanted to use a new {styler} internal (like now). Anyways, I guess it's the proper way to export them so I am open to PRs (unless we want to create more overhead with {styler.infra} package). Note that you need to wait for a {styler} release for your package to be accepted to CRAN. Meanwhile, you could try this workaround

is_function_call <- utils::getFromNamespace("is_function_call", "styler")

@Robinlovelace
Copy link
Author

It sounds like there are 2 main options:

  • PR to the template {styler.yours} package with copy-pasted versions of the functions
  • PR to styler package exporting the functions

As you say both have pros and cons, which works best for you? Happy to try to put in a PR and mild preference for the former option but happy with either and there may be other good options.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Aug 14, 2022

A PR to {styler} is fine. You can hence remove the @keywords internal and add a new {pkgdown} section in the references for these types of functions.

@lorenzwalthert
Copy link
Collaborator

@Robinlovelace if you want this feature, please send a PR.

@Robinlovelace
Copy link
Author

@Robinlovelace if you want this feature, please send a PR.

Thanks for the nudge @lorenzwalthert and see my attempts at implementing this here: #1037

Comments and changes welcome 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants