Moves `{shiny}` to imports and updates examples/vignettes

[averissimo]

Pull Request

• Fixes #529

ℹ️ Note: The package imports all functions from {shiny} via @import shiny {roxygen2} tag, however, there's a mixed bag of prefixed (shiny::fun(...)) and non-prefixed calls (fun(...)) in the (functions') code. Those were not modified. (see discussion)

Changes description

• Remove occurences of shiny:: from R/ code
• Moved {shiny} to Imports
• Replaced shiny functions in examples by
  • add library(shiny) when building a shiny app to run
  • fun -> shiny::fun for others
• Corrects vignettes accordingly

[github-actions[bot]]

Code Coverage Summary

Filename                        Stmts    Miss  Cover    Missing
----------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------------------------------------------------------------
R/calls_combine_by.R                7       0  100.00%
R/choices_labeled.R                49      14  71.43%   25, 36, 41, 51-56, 68, 72-76
R/count_labels.R                   97       0  100.00%
R/filter_panel_api.R               29       1  96.55%   129
R/FilteredData-utils.R             68      25  63.24%   21-24, 27-30, 52-57, 153, 175-184
R/FilteredData.R                  554     219  60.47%   110, 184, 326, 398, 501-510, 533, 554-595, 613-616, 632, 673-706, 721-723, 727-733, 762-790, 813-815, 819-821, 824-835, 839-848, 850-888, 929, 952-974
R/FilteredDataset-utils.R          23       1  95.65%   122
R/FilteredDataset.R               170      61  64.12%   52, 152, 191, 216-273, 312-314
R/FilteredDatasetDataframe.R      121       8  93.39%   87, 148, 158, 234-238
R/FilteredDatasetDefault.R         18       4  77.78%   103-116
R/FilteredDatasetMAE.R            134      37  72.39%   56, 117-122, 161-166, 170-171, 189-211
R/FilterPanelAPI.R                 10       0  100.00%
R/FilterState-utils.R             101       2  98.02%   264, 294
R/FilterState.R                   361      61  83.10%   89, 212, 230-234, 241-242, 256-257, 263-264, 311, 313, 315, 367, 411, 639, 682-705, 715-734, 769-775, 784-790
R/FilterStateChoices.R            336     108  67.86%   310-313, 325, 367, 389-396, 400-417, 445, 458-469, 481-489, 493-522, 543-546, 549-552, 563-586, 599-600, 610
R/FilterStateDate.R               212     129  39.15%   227, 279-436
R/FilterStateDatettime.R          309     199  35.60%   266, 318-549
R/FilterStateEmpty.R               53      31  41.51%   89, 99-104, 117, 129-169
R/FilterStateExpr.R                75      62  17.33%   149-272
R/FilterStateLogical.R            196     144  26.53%   136, 158, 218, 222-406
R/FilterStateRange.R              410     105  74.39%   262, 386, 512-516, 519-529, 532, 544-550, 561-573, 577-587, 591-593, 607-634, 649, 652, 666-683, 718-723, 733-735
R/FilterStates-utils.R             70       9  87.14%   108, 127, 188-194, 216, 245
R/FilterStates.R                  364      30  91.76%   78-82, 191, 314-323, 411-414, 457, 542-546, 591, 712-715
R/FilterStatesDF.R                  5       0  100.00%
R/FilterStatesMAE.R                10       1  90.00%   40
R/FilterStatesMatrix.R              3       0  100.00%
R/FilterStatesSE.R                211     157  25.59%   36, 71-73, 83-85, 109-116, 124-131, 154-302
R/include_css_js.R                  5       5  0.00%    12-16
R/teal_slice.R                    107       4  96.26%   134, 188-189, 200
R/teal_slices.R                    84       5  94.05%   146-151
R/test_utils.R                     21       0  100.00%
R/utils.R                          18       0  100.00%
R/variable_types.R                 15       8  46.67%   44-51
R/zzz.R                            16      16  0.00%    3-46
TOTAL                            4262    1446  66.07%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: bfd6c2b33c12e5ae9376477405b8d2517bd77684

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

[github-actions[bot]]

Unit Tests Summary

  1 files   29 suites   23s :stopwatch: 359 tests 359 :white_check_mark: 0 :zzz: 0 :x: 822 runs  822 :white_check_mark: 0 :zzz: 0 :x:

Results for commit bfd6c2b3.

:recycle: This comment has been updated with latest results.

[averissimo]

Replaced shiny functions in examples by

• add library(shiny) when building a shiny app to run
• fun -> shiny::fun for others

I can easily use library(shiny) for all if that convention is better

[chlebowa]

Since the entire shiny namespace is imported, I believe we should

• not prefix shiny functions in code
• keep prefixes in tests
• add library(shiny) in all exapmles that require it

My reasons are:

1. We have been removing namespace prefixes from examples lately, especially in teal.
2. Attaching other packages in examples is common practice.

[averissimo]

Ok, makes sense I'll make those changes. Hold on on the review then :smile:

[averissimo]

Heads up that the double colon change will create a huge PR to review, what do you think about splitting these in 2?

1. examples/vignettes/DESCRIPTION
2. removing double colon calls to shiny

[averissimo]

By huge it's 79 replacements :sweat_smile:

[m7pr]

I don't think there is any reason for removing double colon calls to shiny in the code. We put package:: prefix also to allow easier code debugging and improve code readability. Even if shiny:: prefix is not needed when we import shiny into NAMESAPCE there are still other benefits of prefixing packages in code.

[m7pr]

Removal of shiny:: prefix for shiny only and leaving package:: prefix for other packages would mix up two conventions. You either list all imported packages and functions in NAMESPACE and do not prefix anything, or you keep NAMESPACE partially filled and you use package:: prefix for everything. I believe our NAMESPACE is to fully filled to cover other packages, that's why we are left with package:: prefix option.

[chlebowa]

Removal of shiny:: prefix for shiny only and leaving package:: prefix for other packages would mix up two conventions.

Hardly. The namespace is imported. We import single functions in several places, e.g. as, sertNames. Would you have every selectInput prefixed? Every div?

[m7pr]

Fair comment! Of course I don't support putting shiny:: before selectInputs and divs. After having seconds thoughts I also don't think we need to put shiny:: prefix before isolate or reactive or reactiveValues. Maybe it's not that bad idea at all.

For the NAMESPACE, just a side note: if you have one function imported for a package, but you use two and both are not prefixed, you will get a warning for the second one that does not have a prefix and is not listed in the NAMESPACE that it can not be found. NAMESPACE in teal.slice doesn't have importFrom(teal.logger, ) and that's why we need to prefix each teal.logger usage.

[averissimo]

We could import only specific heavily used functions and a double colon for the rest.

There are around:

• 259 div calls
• 183 tags$
• 631 isolate()
• 215 reactive()

It could be overkill to replace all these

[averissimo]

But that could be a hard line to define which are heavily used and those that are not.

[chlebowa]

For the NAMESPACE, just a side note: if you have one function imported for a package, but you use two and both are not prefixed, you will get a warning for the second one that does not have a prefix and is not listed in the NAMESPACE that it can not be found.

As expected.

NAMESPACE in teal.slice doesn't have importFrom(teal.logger, ) and that's why we need to prefix each teal.logger usage.

That much is obvious, what's your point?

[m7pr]

We could import only specific heavily used functions and a double colon for the rest.

That's cleaner than @import shiny (everything). I think those used more than 50 times could be a heavily used function

[m7pr]

That much is obvious, what's your point?

Just saying our NAMESPACE file is not filled for the convention where we do not prefix anything. That's all. A side-side-side note :)

[chlebowa]

That much is obvious, what's your point?

Just saying our NAMESPACE file is not filled for the convention where we do not prefix anything. That's all. A side-side-side note :)

@import shiny allows you to use all functions in the package code with no prefixes.> > We could import only specific heavily used functions and a double colon for the rest.

We could import only specific heavily used functions and a double colon for the rest.

That's cleaner than @import shiny (everything). I think those used more than 50 times could be a heavily used function

I disagree. It is cleaner to have consistent code. 50 is an arbitrary number, why not 47? And if I add or remove a usage in the future, do I have to tally and change accordingly? Ridiculous.

[averissimo]

We could import only specific heavily used functions and a double colon for the rest.

That's cleaner than @import shiny (everything). I think those used more than 50 times could be a heavily used function

I would tentatively agree, the caveat is that it would create a *long* NAMESPACE file which is kind of a deal breaker.

Re: this PR

How about:

1. Remove double colon on this PR as it's "only" 79 occurrences of (mostly) isolate, reactive*, render* & co
2. Continue the discussion on the original issue

[m7pr]

I disagree. It is cleaner to have consistent code. 50 is an arbitrary number, why not 47? And if I add or remove a usage in the future, do I have to tally and change accordingly? Ridiculous.

World is not ideal and sometimes you need to come up with an arbitrary number to make things easier. In business it's so-called BUSINESS VALUE based on BUSINESS EXPERIENCE : D and that sometimes needs to be revisited, as all that we do: we do revisit examples and documentation, and we do revisit NAMESPACE and DESCRIPTION.

I would actually revisit @import package function / importFrom(package, ) [those functions treated as heavily used] as often as we do revisit DependsVSImports for a package.

50 is an arbitrary number, why not 47

Rounded numbers are easier to remember. Do we any any more motivation for 50 being better than 47 :D?

[chlebowa]

@m7pr Either you're trolling or you are assuming the worst practices of business that we always complain about. Did you skip this sentence on purpose?

And if I add or remove a usage in the future, do I have to tally and change accordingly?

@averissimo I agree, go ahead :+1:

[m7pr]

This BUSINESS VALUE was a joke (added : D).

And if I add or remove a usage in the future, do I have to tally and change accordingly?

I think seasonal revisit should be enough. And I think that's what we do with our revisits.

[m7pr]

If it's only 79 occurrences then go ahead and remove with gsub

[averissimo]

PR is now ready for review :smile:

Removed all double colon in R/ code and added a few missing from tests/

[pawelru]

not prefix shiny functions in code

Hmmm... shiny is on the same level as checkmate (and any other package in Imports) - why don't we prefix shiny calls and we do prefix checkmate (and all the other imported pkgs)? I always look for consistency in our codebase and for this reason I think we should do prefix inside the codebase.

It could be overkill to replace all these

This might be true. From my perspective the most important is DESC file and examples. Within the package, prefixed or not - the code remains fully functional (unless namespace collision which I hope is not there). Therefore I propose to do this in two parts if for some reason (e.g. approaching release) this cannot be done all at once and prefix all later on in the second step. WDYT?

[chlebowa]

not prefix shiny functions in code

Hmmm... shiny is on the same level as checkmate (and any other package in Imports) - why don't we prefix shiny calls and we do prefix checkmate (and all the other imported pkgs)? I always look for consistency in our codebase and for this reason I think we should do prefix inside the codebase.

checkmate is in Imports but we don't @import checkmate.

An argument can be made for R6 but we can defer that because it does is not connected to the dependency structure between our packages.

[pawelru]

not prefix shiny functions in code

Hmmm... shiny is on the same level as checkmate (and any other package in Imports) - why don't we prefix shiny calls and we do prefix checkmate (and all the other imported pkgs)? I always look for consistency in our codebase and for this reason I think we should do prefix inside the codebase.

checkmate is in Imports but we don't @import checkmate.

An argument can be made for R6 but we can defer that because it does is not connected to the dependency structure between our packages.

So #' @import shiny is done to make shiny function available without prefix. I feel it's more a direct consequence and not the reason of the decision of not to prefix. The question is still - why don't we prefix (and have this import statement as a consequence)? (note we can still prefix and have import) (a side note, reading this now - this)

[averissimo]

In some examples isolate can be replaced with reactiveConsole(TRUE). What do you think?

On one hand, it might induce the developer into thinking that it doesn't need reactivity, on the other many of these examples will be used within a reactive environment anyway, so it's a realistic snippet.

I'm not a fan of the description in ?reactiveConsole though.

This is an experimental feature that allows you to enable reactivity at the console, for the purposes of experimentation and learning.

I leaning toward keeping it as it is, but if you think it would be better, then I won't oppose

[chlebowa]

So #' @import shiny is done to make shiny function available without prefix.

*all functions

why don't we prefix

Because there are too many. That's how I see it, at least.

[m7pr]

reactiveConsole(TRUE) call will change user's session settings : P maybe that's a point for not using that in examples? unless we end them with reactiveConsole(FALSE)

[chlebowa]

reactiveConsole(TRUE) call will change user's session settings : P maybe that's a point for not using that in examples? unless we end them with reactiveConsole(FALSE)

That was the implication, yes.

I'm not a fan of the description in ?reactiveConsole though.

Fair enough, let's stick to isolate.

[pawelru]

why don't we prefix

Because there are too many. That's how I see it, at least.

That's okey. I guess we all expected this. I mentioned earlier that this can be done in two step approach if that's too much for the tight deadlines. In general I feel that prefixing is much more safer way of writing the code. I like the reasoning I have found (lined above): while this is okey right now, it does not guarantee that in the future packages will export objects with the same name and there will be name collision. Prefixing is a safeguard for such situation.

Coming back to this PR All the changes are good but removing prefixes from the codebase feels like a adding more into a lot of work anyway. I suggest to revert those.

[chlebowa]

@pawelru By that rationale we should prefix every div and every observeEvent. Is this what you want?

[pawelru]

@pawelru By that rationale we should prefix every div and every observeEvent. Is this what you want?

Yessir. For the same reason we have (had?) prefixed isolate(), reactive() etc. Consistency - I'm looking for consistency. Regardless if it's 3 or 33 occurrences.

[chlebowa]

@pawelru By that rationale we should prefix every div and every observeEvent. Is this what you want?

Yessir.

I vehemently oppose this.

For the same reason we have (had?) prefixed isolate(), reactive() etc. Consistency - I'm looking for consistency.

We do have consistency now (with this PR). Import nothing - prefix, import one function - don't prefix that, import whole namepsace - prefix nothing.

Regardless if it's 3 or 33 occurrences.

I don't know what this number means.

[pawelru]

@pawelru By that rationale we should prefix every div and every observeEvent. Is this what you want?

Yessir.

I vehemently oppose this.

So we have a disagreement then. Will I insist - not really. Not for this. This is a relatively small thing anyway. Will stick to what you guys agree on. But still I can't help having a feeling that removing prefixes is a step back in some sense and makes this pkg code more vulnerable for potential namespace collision.

For the same reason we have (had?) prefixed isolate(), reactive() etc. Consistency - I'm looking for consistency.

We do have consistency now (with this PR). Import nothing - prefix, import one function - don't prefix that, import whole namepsace - prefix nothing.

OK so there's a logic in this than. I missed this somehow.

Regardless if it's 3 or 33 occurrences.

I don't know what this number means.

These have no meaning. It's just to highlight that number of occurrences is irrelevant.

[averissimo]

I favor the direction of using prefix on {teal.*} code. Shiny's namespace is just too big (277 exports).

I revisited the "prefix" subject today when observing shiny::code() and shiny::includeCSS and teal::include_css_files on a PR (getting a chill) and took another look at occurences

I found that I was grossly overestimating occurences on a comment before (by including vignettes and rstudio hidden folders on the grep).

We could open room for a hybrid approach that would import a select set of functions (UI and/or heavily used functions).

# {shiny} usage in {teal} (there could be false positives here `tags$div(` will appear 2x under `tags` and `div`)
# github hash used 1a9976664
26 tags
17 div
16 validate
15 need
14 observeEvent
13 moduleServer
13 icon
12 reactive
12 isolate
10 NS
10 is.reactive
# expand below to see all <10

Occurences of {shiny} namespace in teal

```bash #!/bin/bash if [ "$2" == "wc" ];then echo $(grep -RE "(($1\()|($1\\$))" R | grep -vE "(::${1})|([a-zA-Z0-9_.]$1)" | grep -vE ":#'" | wc -l) $1 else grep -RE "(($1\()|($1\\$))" R | grep -vE "(::${1})|([a-zA-Z0-9_.]$1)" | grep -vE ":#'" | grep -E --color "$1" fi # 1. Create "shiny_namespace.bash" with script above # 2. create a "shiny_namespace.txt" with all functions to check" # 3. run: $ for word in $(cat shiny_namespace.txt); do bash shiny_namespace.bash $word wc; done | grep -v "^0 " | sort -h -r ``` ```r 26 tags 17 div 16 validate 15 need 14 observeEvent 13 moduleServer 13 icon 12 reactive 12 isolate 10 NS 10 is.reactive 7 showNotification 7 reactiveVal 7 p 6 tagList 5 singleton 5 showModal 5 modalDialog 5 modalButton 4 renderUI 4 removeModal 4 eventReactive 4 column 4 actionLink 4 actionButton 3 updateTextInput 3 uiOutput 3 reactiveValues 3 hr 2 textInput 2 span 2 fluidRow 1 verbatimTextOutput 1 updateSelectInput 1 textOutput 1 tagAppendChild 1 tabPanel 1 tableOutput 1 selectInput 1 req 1 renderText 1 renderTable 1 renderPrint 1 removeUI 1 reactiveValuesToList 1 pre 1 insertUI 1 h5 1 h2 1 getDefaultReactiveDomain 1 fluidPage 1 fileInput 1 downloadLink 1 downloadHandler 1 conditionalPanel 1 code ```

[chlebowa]

Only 12 reactive and only one req? Only 14 observeEvent?? I can't believe it :raised_back_of_hand:

[averissimo]

The grep script isn't perfect, but it's in the ballpark