Closes #2143 Loosen `restrict_derivation()` assertion #2144

zdz2101 · 2023-09-28T19:14:17Z

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the devel branch until you have checked off each task.

github-actions · 2023-09-28T19:19:37Z

Package	Line Rate	Health
admiral	98%	✔
Summary	98% (4386 / 4455)	✔

Merge branch 'main' into 2143_restrict_derivation_lighten_assertion # Conflicts: # DESCRIPTION # NEWS.md

bms63

Do any of the vignettes or documentation need to be changed for this update? I thought we said this wasn't allowed somewhere?

zdz2101 · 2023-10-06T19:45:05Z

Do any of the vignettes or documentation need to be changed for this update? I thought we said this wasn't allowed somewhere?

Don't recall that portion, I imagine we should be okay, I would not say its particularly intuitive to call mutate using this either, had a hard time coming up with an appropriate test, I will say I did miss a roxygen blurb

zdz2101 · 2023-10-06T19:46:43Z

R/restrict_derivation.R

@@ -12,7 +12,7 @@
 #'   The function must provide the `dataset` argument and all arguments
 #'   specified in the `params()` objects passed to the `arg` argument.
 #'
-#'   Please note that it is not possible to specify `{dplyr}`
+#'   Please note that it is not advised to specify `{dplyr}`


@bms63 Was it this that you meant?

Umm...is this saying not to do dplyr::mutate() ?

Well I'm not quite sure the origin story of this one, but all we wanted to do was loosen up the restriction such that we allow functions that don't necessarily have "dataset" as an argument. Sounds like there was a firm decision to outright not-allow dplyr functions or at least explicitly say they don't work since they don't file under the original restriction, but after lifting it, might lead to loosened/unintended-usage by users.

@bundfussr any memory of this one?

As far as I remember this sentence was added after the last clean-up to point out that dplyr functions can not be used. As this is changed now, we should remove the sentence completely.

bundfussr · 2023-10-10T09:50:17Z

NEWS.md

@@ -4,10 +4,13 @@

 ## Updates of Existing Functions

+- `restrict_derivation()` now allows `dplyr` functions like `mutate` in the `derivation argument (#2143)


Should be `{dplyr}` instead of `dplyr`. Then it is autolinked.

bundfussr · 2023-10-10T11:24:51Z

R/restrict_derivation.R

-  if (!is.null(args)) {
-    assert_function(derivation, names(args))
-  }


I wonder if we should keep this call and update assert_function() such that it handles functions with ... argument. I.e., if the functions provides the ... argument, all arguments are accepted.

@zdz2101 , @bms63 , what do you think?

I don't know if that is necessary. I feel like the original purpose of the assertion, was to check if all the appropriate necessary arguments were provided for that specific function. And the developer would likely know what is needed for that hard-check when using the assertion.

Many functions that use ... still have other necessary arguments anyway, so they can also just run assert_function(dev_fun) without checking params, to ensure it is a function. The remainder of that function, dev_fun in this case, should provide the appropriate errors/message handling if something is plugged in incorrectly.

I think the intention of the check was to ensure that all arguments specified for args are arguments of the function specified for derivation.

Well I suppose it is a relatively short implementation, I can make an appropriate issue for it

…github.com/pharmaverse/admiral into 2143_restrict_derivation_lighten_assertion

zdz2101 · 2023-10-10T17:35:14Z

Closing in favor of #2164

Zelos Zhu added 2 commits September 28, 2023 17:56

feat: #2145 lighten restriction rough draft

15c8098

feat: #2143 run addin and add news blurb

d55b000

zdz2101 linked an issue Sep 28, 2023 that may be closed by this pull request

Feature Request: Lighten up restrictions of restrict_derivation() #2143

Closed

Zelos Zhu added 3 commits October 6, 2023 17:57

merge main

1850984

Merge branch 'main' into 2143_restrict_derivation_lighten_assertion # Conflicts: # DESCRIPTION # NEWS.md

missed a conflict

6bb3e37

missed another

3d81518

zdz2101 requested review from bms63 and bundfussr October 6, 2023 17:59

zdz2101 marked this pull request as ready for review October 6, 2023 18:00

bms63 reviewed Oct 6, 2023

View reviewed changes

chore: #2143 minor edit to roxygen documentation

7432866

zdz2101 commented Oct 6, 2023

View reviewed changes

zdz2101 and others added 2 commits October 6, 2023 12:46

Merge branch 'main' into 2143_restrict_derivation_lighten_assertion

b5945b6

Merge branch 'main' into 2143_restrict_derivation_lighten_assertion

159a6be

bundfussr reviewed Oct 10, 2023

View reviewed changes

Zelos Zhu added 2 commits October 10, 2023 13:14

chore: #2143 adopt recommended changes

54dca70

Merge branch '2143_restrict_derivation_lighten_assertion' of https://…

b5d91e3

…github.com/pharmaverse/admiral into 2143_restrict_derivation_lighten_assertion

This was referenced Oct 10, 2023

Feature Request: Enhance assert_function() to accomodate for ... pharmaverse/admiraldev#339

Closed

Closes #2143 Loosen restrict_derivation() assertion #2164

Merged

zdz2101 closed this Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #2143 Loosen `restrict_derivation()` assertion #2144

Closes #2143 Loosen `restrict_derivation()` assertion #2144

zdz2101 commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023 •

edited

Loading

bms63 left a comment

zdz2101 commented Oct 6, 2023

zdz2101 Oct 6, 2023

bms63 Oct 8, 2023

zdz2101 Oct 9, 2023 •

edited

Loading

bms63 Oct 9, 2023

bundfussr Oct 10, 2023

bundfussr Oct 10, 2023

bundfussr Oct 10, 2023

zdz2101 Oct 10, 2023

bundfussr Oct 10, 2023

zdz2101 Oct 10, 2023

zdz2101 commented Oct 10, 2023

		@@ -4,10 +4,13 @@

		## Updates of Existing Functions

		- `restrict_derivation()` now allows `dplyr` functions like `mutate` in the `derivation argument (#2143)

Closes #2143 Loosen restrict_derivation() assertion #2144

Closes #2143 Loosen restrict_derivation() assertion #2144

Conversation

zdz2101 commented Sep 28, 2023 • edited Loading

github-actions bot commented Sep 28, 2023 • edited Loading

bms63 left a comment

Choose a reason for hiding this comment

zdz2101 commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdz2101 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdz2101 commented Oct 10, 2023

Closes #2143 Loosen `restrict_derivation()` assertion #2144

Closes #2143 Loosen `restrict_derivation()` assertion #2144

zdz2101 commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023 •

edited

Loading

zdz2101 Oct 9, 2023 •

edited

Loading