Skip to content
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

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

Closed
3 tasks done
pawelru opened this issue Mar 21, 2024 · 8 comments
Closed
3 tasks done

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

pawelru opened this issue Mar 21, 2024 · 8 comments
Assignees
Labels
core enhancement New feature or request
Milestone

Comments

@pawelru
Copy link
Contributor

pawelru commented Mar 21, 2024

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

  • I agree to follow this project's Code of Conduct.

Contribution Guidelines

  • I agree to follow this project's Contribution Guidelines.

Security Policy

  • I agree to follow this project's Security Policy.
@pawelru pawelru added the enhancement New feature or request label Mar 21, 2024
@donyunardi donyunardi added this to the teal v0.16.0 milestone Mar 22, 2024
@m7pr m7pr self-assigned this May 8, 2024
@m7pr
Copy link
Contributor

m7pr commented May 8, 2024

@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
Copy link
Contributor Author

pawelru commented May 8, 2024

Yes. That's correct.

@m7pr
Copy link
Contributor

m7pr commented May 8, 2024

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
Copy link
Contributor Author

pawelru commented May 8, 2024

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
Copy link
Contributor

m7pr commented May 8, 2024

@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
Copy link
Contributor

gogonzo commented May 8, 2024

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
Copy link
Contributor

m7pr commented May 8, 2024

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

@m7pr
Copy link
Contributor

m7pr commented May 8, 2024

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!

insightsengineering/teal.modules.general#749
insightsengineering/teal.modules.clinical#1180
insightsengineering/teal.osprey#264
insightsengineering/teal.goshawk#273

m7pr added a commit to insightsengineering/teal.osprey that referenced this issue May 10, 2024
Part of insightsengineering/teal#1176

---------

Signed-off-by: kartikeya kirar <[email protected]>
Signed-off-by: Marcin <[email protected]>
Co-authored-by: unknown <[email protected]>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
m7pr added a commit to insightsengineering/teal.goshawk that referenced this issue May 10, 2024
m7pr added a commit to insightsengineering/teal.modules.general that referenced this issue May 10, 2024
@m7pr m7pr closed this as completed May 10, 2024
m7pr added a commit to insightsengineering/teal.modules.clinical that referenced this issue May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants