search

insightsengineering / teal

Exploratory Web Apps for Analyzing Clinical Trial Data
https://insightsengineering.github.io/teal/
Other
168 stars 33 forks source link

[Feature Request]: Remove "Show warnings" feature #1176

Closed pawelru closed 2 months ago

pawelru commented 4 months ago

Feature description

As we move away from chunks in favour of qenv I see little point of having this functionality. I propose to remove it complitely.

Code of Conduct

Contribution Guidelines

Security Policy

m7pr commented 2 months ago

@pawelru just to clarify, we are planning to remove Show Warnings verbatim popup from all modules, which e.g. for tm_missing_data would be the removal of:

And that happens for all modules that we have?

image
pawelru commented 2 months ago

Yes. That's correct.

m7pr commented 2 months ago

So I guess we won't need teal.code::get_warnings() https://github.com/insightsengineering/teal.code/blob/main/R/qenv-get_warnings.R and handling warnings during teal.code::eval_code() https://github.com/insightsengineering/teal.code/blob/main/R/qenv-eval_code.R#L68-L71 ? If this is not used by any module

pawelru commented 2 months ago

So I guess we won't need teal.code::get_warnings() https://github.com/insightsengineering/teal.code/blob/main/R/qenv-get_warnings.R and handling warnings during teal.code::eval_code() https://github.com/insightsengineering/teal.code/blob/main/R/qenv-eval_code.R#L68-L71 ? If this is not used by any module

This I don't know. It's quite likely but you would have to check it.

m7pr commented 2 months ago

@gogonzo do you see any usage of @warnings slot in qenv/teal_data objects if we stop exposing warnings via teal.code::get_warnings in modules ?

gogonzo commented 2 months ago

So I guess we won't need teal.code::get_warnings() https://github.com/insightsengineering/teal.code/blob/main/R/qenv-get_warnings.R and handling warnings during teal.code::eval_code() https://github.com/insightsengineering/teal.code/blob/main/R/qenv-eval_code.R#L68-L71 ? If this is not used by any module

I think it makes sense to keep get_warnings in teal.data. Somebody might need it in custom modules or in its custom framework.

m7pr commented 2 months ago

in teal.data or in teal.code :)? get_warnings was introduced in teal.code

m7pr commented 2 months ago

I created 4 PRs to remove this functionality from our modules.

@kartikeyakirar , since @gogonzo and @pawelru are away due to the banking holiday tomorrow, would you mind to review? The goal was to remove Show Warnings from the UI (hence the server as well) of all modules that we have. Thanks!

https://github.com/insightsengineering/teal.modules.general/pull/749 https://github.com/insightsengineering/teal.modules.clinical/pull/1180 https://github.com/insightsengineering/teal.osprey/pull/264 https://github.com/insightsengineering/teal.goshawk/pull/273