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

disable total column if not in parallel #1087

Merged
merged 4 commits into from
Mar 1, 2024
Merged

disable total column if not in parallel #1087

merged 4 commits into from
Mar 1, 2024

Conversation

pawelru
Copy link
Contributor

@pawelru pawelru commented Feb 27, 2024

TLGCatalog EGT01 filed because of this.

Talked with @Melkiades and it seems that summarize_colvars() does not play nicely together with add_overall_col() before. Hence putted this in the if condition.

You can test it as follows:

  • pkgload::dev_example("tm_t_summary_by")
  • assure "Add All Patients column" is TRUE
  • turn on "Show summarize variables in parallel"

Currently on main it's (gracefully) failing. After this fix, all patients flag is silently ignored.

@pawelru pawelru added the sme label Feb 27, 2024
Copy link
Contributor

github-actions bot commented Feb 27, 2024

Unit Tests Summary

  1 files   33 suites   2s ⏱️
150 tests 150 ✅   0 💤 0 ❌
282 runs  170 ✅ 112 💤 0 ❌

Results for commit 5c1d9f7.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@Melkiades Melkiades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! I am just wondering if we do not need a refactoring of that function

@pawelru pawelru merged commit f46fcf6 into main Mar 1, 2024
21 checks passed
@pawelru pawelru deleted the fix_tm_t_summary branch March 1, 2024 09:50
@pawelru
Copy link
Contributor Author

pawelru commented Mar 1, 2024

I am just wondering if we do not need a refactoring of that function

You tell me :)

pawelru added a commit to insightsengineering/tlg-catalog that referenced this pull request Mar 1, 2024
Keeping it as a draft for the time being.

There are a few PRs that needs to be merged to make it pass:

- [x] insightsengineering/teal.data#301
- [x]
insightsengineering/teal.modules.clinical#1087

Locally, when using above fixes, I get the whole catalog rendered. 
This does not mean that stable will be working - that needs additional
work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants