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

Expose internals used with other style guides #1037

Closed
wants to merge 39 commits into from

Conversation

Robinlovelace
Copy link

@Robinlovelace Robinlovelace commented Oct 23, 2022

Closes #974.

Progress tracker

  • Export compute_parse_data_nested() helper
  • Export is_*() helpers
  • Add examples to new exports
  • Check function and argument names are okay (since these are now user-facing)
  • Code clean-up
  • Code review

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2022

Codecov Report

Merging #1037 (287daff) into main (a45f667) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 287daff differs from pull request most recent head f6bdea9. Consider uploading reports for the commit f6bdea9 to get more accurate results

@@            Coverage Diff             @@
##             main    #1037      +/-   ##
==========================================
- Coverage   91.06%   91.05%   -0.02%     
==========================================
  Files          46       46              
  Lines        2687     2682       -5     
==========================================
- Hits         2447     2442       -5     
  Misses        240      240              
Impacted Files Coverage Δ
R/detect-alignment.R 97.80% <ø> (ø)
R/initialize.R 96.96% <ø> (ø)
R/nest.R 100.00% <ø> (ø)
R/nested-to-tree.R 92.85% <ø> (ø)
R/style-guides.R 99.43% <ø> (ø)
R/utils-navigate-nest.R 81.48% <ø> (ø)
R/expr-is.R 86.11% <100.00%> (ø)
R/rules-indention.R 100.00% <100.00%> (ø)
R/rules-line-breaks.R 100.00% <100.00%> (ø)
R/rules-spaces.R 98.29% <100.00%> (-0.05%) ⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if a54e5bb is merged into main:

  •   :rocket:cache_applying: 35.1ms -> 32.2ms [-11.2%, -5.44%]
  •   :ballot_box_with_check:cache_recording: 1.06s -> 1.06s [-1.07%, +1.29%]
  •   :ballot_box_with_check:without_cache: 2.67s -> 2.68s [-0.67%, +1.09%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if ecd0433 is merged into main:

  •   :ballot_box_with_check:cache_applying: 33ms -> 33.8ms [0%, +4.65%]
  •   :ballot_box_with_check:cache_recording: 942ms -> 942ms [-0.79%, +0.8%]
  •   :ballot_box_with_check:without_cache: 2.38s -> 2.37s [-1.07%, +0.33%]

Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil left a comment

Choose a reason for hiding this comment

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

All user-facing functions should also include examples in the documentation.

@Robinlovelace
Copy link
Author

All user-facing functions should also include examples in the documentation.

Thanks for the review @IndrajeetPatil. Do you have examples of the kind of situations when these functions would be used? I am not as familiar with the package as you and the other developers so support here would be greatly appreciated.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 7652952 is merged into main:

  •   :ballot_box_with_check:cache_applying: 29.7ms -> 29.8ms [-3.26%, +4.37%]
  •   :ballot_box_with_check:cache_recording: 820ms -> 829ms [-0.88%, +3%]
  •   :ballot_box_with_check:without_cache: 2.06s -> 2.04s [-3.07%, +0.74%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert lorenzwalthert changed the title Update description with remotes Expose internals used with other style guides Oct 24, 2022
@IndrajeetPatil
Copy link
Collaborator

@Robinlovelace Sure! Will have a look either today or tomorrow.

@lorenzwalthert
Copy link
Collaborator

Please add a roxygen family with #' @family third-party style guide helpers (or similar) and list these also separately in the {pkgdwn} index at the bottom with has_concept().

@Robinlovelace
Copy link
Author

Thanks for the suggestion @lorenzwalthert, PR just updated. I think I may struggle with the examples so any help on those greatly appreciated.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 1c84001 is merged into main:

  •   :ballot_box_with_check:cache_applying: 37ms -> 37.2ms [-2.77%, +3.89%]
  •   :ballot_box_with_check:cache_recording: 1.11s -> 1.12s [-1.51%, +3.34%]
  •   :ballot_box_with_check:without_cache: 2.84s -> 2.86s [-1.45%, +2.99%]

Further explanation regarding interpretation and methodology can be found in the documentation.

R/expr-is.R Outdated Show resolved Hide resolved
R/expr-is.R Outdated Show resolved Hide resolved
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@lorenzwalthert
Copy link
Collaborator

Note sure we should remove #' @keywords internal, as this hides the functions from the package index in the help file browser (see screenshot below). I think since 99% of all users won't need the helpers we export in this PR, shouldn't we hide the functions in the index?
Screenshot 2022-10-25 at 20 51 19

@Robinlovelace
Copy link
Author

I think since 99% of all users won't need the helpers we export in this PR, shouldn't we hide the functions in the index?

👍 from me

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 287daff is merged into main:

  •   :rocket:cache_applying: 24.4ms -> 24ms [-3.03%, -0.39%]
  •   :ballot_box_with_check:cache_recording: 756ms -> 757ms [-0.12%, +0.35%]
  •   :rocket:without_cache: 1.91s -> 1.9s [-0.63%, -0.13%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil
Copy link
Collaborator

think since 99% of all users won't need the helpers we export in this PR, shouldn't we hide the functions in the index?

@lorenzwalthert I am not sure if I agree. Although they might not be useful for most users, they are exported and should appear in PDF manual, in help pages, in pkgdown website, etc. E.g., most {dplyr} users will never need to use dplyr::ident(), but docs for this function are included nonetheless.

With #' @keywords internal, the docs will be hidden in the pkgdown website as well. Given that other package developers are also users and need some docs (with examples) to understand how these functions work, they should be included on the website.

We have nothing to lose and developer-users have much to gain by including these functions in the docs.

WDYT?

@lorenzwalthert
Copy link
Collaborator

ok, thanks for the {dplyr} comparison. I don't think internals have to be hidden from {pkgdwn}, i.e. {styler} internals also have a html page (but no links there currently). I am fine with either way.

@IndrajeetPatil
Copy link
Collaborator

Sorry, @Robinlovelace, but the merge conflicts here got too much to resolve, so I have just created a new PR, which is cleaner and should be a bit easier to work with.

Closing this PR in favour of that one.

@Robinlovelace
Copy link
Author

Fine by me. Good luck with it @IndrajeetPatil !

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.

Export of non-exported functions to enable other style guides to pass CRAN checks
4 participants