-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Updates "Decorators" to use name-based execution and new wrappers #812
Updates "Decorators" to use name-based execution and new wrappers #812
Conversation
Hey @averissimo this is sooooooooooooo excellent and fantastic work! Keep it up! It was a pleasure to review and check changes. chapeau bas! When reviewing I tried to apply changes to tmc modules. I am having issues with |
@@ -0,0 +1,30 @@ | |||
# nocov start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://roxygen2.r-lib.org/articles/reuse.html?q=template#superseded
man-roxygen
folder is a very old way of placing the template, if we want to keep using templates we should move to man/roxygen
folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@averissimo should we create a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other repos yes, although it's a low priority until it's really deprecated (not only twice superseded)
- But still a priority IMO, as it's VERY weird to have
man-roxygen
folder on the root folder
For this one, depends on this PR and how we deal with @param decorators
if we keep as is, the change can tag along.
#' @param ggplot2_args `r roxygen_ggplot2_args_param("Elbow plot", "Circle plot", "Biplot", "Eigenvector plot")` | ||
#' @param decorators `r roxygen_decorators_param("tm_a_pca")` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this sorcery : p?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the recommended way for dealing with roxygen2
tags.
It's all about having the tm_<module_name>
as seen in image below.
It's nice to have, but I'm more than happy to revert this and keep it simple as it was (ggplot2_args back to template and shared @param decorators
with "See "Decorating tm_<module_name> below"
or an equivalent generic text
#' @param decorators `r lifecycle::badge("experimental")` (`list` of `teal_transform_module` or `NULL`) optional, | ||
#' if not `NULL`, decorator for tables or plots included in the module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is removed? This was needed so that in each module we added decorators
parameter automatically to the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's added per module. this comment is a byproduct of this.
Which allows to have the module name specific text. It's a small issue, but it seems odd to have See tm_<module_name>
in the documentation for the function.
This is a minor thing that we can remove and go back to a shared parameter.
For instance in the tm_missing_data
.
+#' @param decorators `r roxygen_decorators_param("tm_missing_data")`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this is an amazing work! Well done my man.
I left a couple of questions that are really minor.
Let's get this merged, and let's start manually testing on the feature
branch
Co-authored-by: Marcin <[email protected]> Signed-off-by: André Veríssimo <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: André Veríssimo <[email protected]>
Co-authored-by: Marcin <[email protected]> Signed-off-by: André Veríssimo <[email protected]>
@m7pr The open comments are related to the same. I'm merging this PR and moving that conversation to the other PR so it doesn't block review |
Modules
1 object
2 objects
3 objects
4 objects
Not applicable
tm_file_viewertm_front_pagetm_variable_browserChanges description
ui_decorate_teal_data
andsrv_decorate_teal_data
wrapper to simplify codedecorators
argument in module See this commentApp with all modules (WIP)
Working example