-
-
Notifications
You must be signed in to change notification settings - Fork 15
Updates "Decorators" to use name-based execution and new wrappers #812
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
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-roxygenfolder 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")`
m7pr
left a comment
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_dataandsrv_decorate_teal_datawrapper to simplify codedecoratorsargument in module See this commentApp with all modules (WIP)
Working example