shinyvalidate improvements

[chlebowa]

Closes this issue

Following the introduction of validate_inputs to teal by #199,
this PR:

• changes all possible input validations from validate calls to shinyvalidate input validators
• passes all validators to validate_input funcitons

[BLAZEWIM]

Concerning gather_fails:

Like the idea and the example app. My comments / questions in the conversations.
It is only draft so I do not care about spelling or comments.
Are all those checkmate's needed? The number of developers that can make those mistakes is limited.

[chlebowa]

All modules have been modified.

[chlebowa]

Concerning gather_fails:

Like the idea and the example app. My comments / questions in the conversations. It is only draft so I do not care about spelling or comments. Are all those checkmate's needed? The number of developers that can make those mistakes is limited.

I add checkmates by habit in case something is ever called programmatically.

[donyunardi]

Sorry, maybe I shouldn't worry about the misspelling but I thought I just pointed out so we don't forget about it.

Overall, I think collating the validation messages is a great idea and will improve user's experience!

One scenario that I could recall recently is when a user got frustrated as he got into another issue after fixing one issue.
In this case, the data structure was not proper for the module (i.e. levels is expected on several columns) but I feel that expectation can be set should all validation warnings appear at once.

I think you pointed out that gather_fails should be in teal which I agree.

[gogonzo]

👍

[chlebowa]

I decided to add debouncing to all_q reactives in modules where the output takes long to update, which was most of them.

[github-actions]

Code Coverage Summary

Filename                    Stmts    Miss  Cover    Missing
------------------------  -------  ------  -------  --------------
R/tm_g_ae_oview.R             247     247  0.00%    110-390
R/tm_g_ae_sub.R               316     316  0.00%    58-410
R/tm_g_butterfly.R            385     385  0.00%    124-549
R/tm_g_decorate.R              46      46  0.00%    18-98
R/tm_g_events_term_id.R       296     296  0.00%    63-397
R/tm_g_heat_bygrade.R         318     318  0.00%    164-517
R/tm_g_patient_profile.R      708     708  0.00%    205-978
R/tm_g_spiderplot.R           321     321  0.00%    83-449
R/tm_g_swimlane.R             375     375  0.00%    120-552
R/tm_g_waterfall.R            422     422  0.00%    113-586
R/utils.R                      37      32  13.51%   32-91, 124-131
R/zzz.R                         1       1  0.00%    2
TOTAL                        3472    3467  0.14%

Diff against main

Filename                    Stmts    Miss  Cover
------------------------  -------  ------  --------
R/tm_g_ae_oview.R              +5      +5  +100.00%
R/tm_g_ae_sub.R                -1      -1  +100.00%
R/tm_g_butterfly.R             +8      +8  +100.00%
R/tm_g_events_term_id.R        +2      +2  +100.00%
R/tm_g_heat_bygrade.R         +35     +35  +100.00%
R/tm_g_patient_profile.R      -78     -78  +100.00%
R/tm_g_spiderplot.R           +15     +15  +100.00%
R/tm_g_swimlane.R              +4      +4  +100.00%
R/tm_g_waterfall.R            +21     +21  +100.00%
TOTAL                         +11     +11  -0.00%

Results for commit: a47863c

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[BLAZEWIM]

One minor thing I noticed that is slightly (5%) annoying: uppercases and lowercases in labels. For example, in spiderplot, we have "Variable" used everywhere, while in patient profile it is "variable". It also goes for other packages. I noticed that developers usually just go with the convention used by labels when they create warning message, and it makes sense, but it could be managed on some lazy day.

[chlebowa]

One minor thing I noticed that is slightly (5%) annoying: uppercases and lowercases in labels. For example, in spiderplot, we have "Variable" used everywhere, while in patient profile it is "variable". It also goes for other packages. I noticed that developers usually just go with the convention used by labels when they create warning message, and it makes sense, but it could be managed on some lazy day.

I was equally annoyed by this but assumed it was due to independent design decisions by different module developers.

[BLAZEWIM]

So the only problem is that the check() output is not full of no visible binding for global variable notes, we could use tidyselect:: all_of, but that will add a dependency...
if we are cool with ignoring notes, it is ok.

[chlebowa]

So the only problem is that the check() output is not full of no visible binding for global variable notes, we could use tidyselect:: all_of, but that will add a dependency... if we are cool with ignoring notes, it is ok.

We decided to ignore them.

[BLAZEWIM]

ok