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

Document imports in a single file #1060

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Document imports in a single file #1060

merged 3 commits into from
Nov 15, 2022

Conversation

IndrajeetPatil
Copy link
Collaborator

Note that the NAMESPACE file remains unchanged.

This is similar to the idiom utilized by {usethis} package.

@github-actions
Copy link
Contributor

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

  •   :ballot_box_with_check:cache_applying: 23.6ms -> 23.7ms [-0.14%, +0.68%]
  •   :ballot_box_with_check:cache_recording: 742ms -> 741ms [-0.55%, +0.35%]
  •   :ballot_box_with_check:without_cache: 1.87s -> 1.87s [-0.5%, +0.14%]

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

@lorenzwalthert
Copy link
Collaborator

Ok. I am not sure that’s the best way though. Just as context: at the very beginning, we imported all objects on a function basis. That’s why there are many duplicated statements. It reminds me now of the Python import style. The good thing about the duplication is that if a function call looks unknown, you can see quickly check the roxygen comments to know where it comes from. But since we anyways mostly imported functions that developers know, and prefixed the more exotic lines with ::, I guess this argument is less strong.

@lorenzwalthert
Copy link
Collaborator

I guess the PR makes sense mostly though. Maybe some of the functions that are just used once and that are more exotic like as_mapper() could also be prefixed with ::, although this might have minor Speedo implications.

Copy link
Collaborator

@lorenzwalthert lorenzwalthert left a comment

Choose a reason for hiding this comment

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

write.table and capture.output could be prefixed, these are not used at runtime anyways, only for testing.

@@ -18,15 +18,24 @@
#' style_text("a%>%b; a", scope = "tokens")
"_PACKAGE"

if (getRversion() >= "2.15.1") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This conditional is unnecessary since it will always be true.

@IndrajeetPatil
Copy link
Collaborator Author

Maybe some of the functions that are just used once and that are more exotic like as_mapper() could also be prefixed with ::

I have done so now.

although this might have minor Speedo implications

Each :: call costs roughly 120 nanoseconds, so I don't think the performance should degrade in any significant way.

@github-actions
Copy link
Contributor

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

  •   :ballot_box_with_check:cache_applying: 23.3ms -> 23.4ms [-0.1%, +0.65%]
  •   :ballot_box_with_check:cache_recording: 723ms -> 722ms [-0.58%, +0.08%]
  •   :ballot_box_with_check:without_cache: 1.83s -> 1.83s [-0.81%, +0.58%]

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

@IndrajeetPatil IndrajeetPatil merged commit acfb42a into main Nov 15, 2022
@IndrajeetPatil IndrajeetPatil deleted the single_pkg_namespace branch November 15, 2022 09:08
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.

2 participants