-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
736 Allow pure custom card functions in modules #742
Open
chlebowa
wants to merge
18
commits into
insightsengineering:main
Choose a base branch
from
chlebowa:in_teal.reporter@custom_card_functions@main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
59da2e1
add hydrate_function
chlebowa 55dc29e
upgrade rlang dependency
chlebowa e2a1a40
isolate card function and add arguments in tm_g_distribution
chlebowa 676b2af
amend documentation
chlebowa 180be37
amend NEWS
chlebowa d13fb79
add missing roxygen tag
chlebowa 377da56
Merge branch 'main' into custom_card_functions@main
chlebowa e33a666
Merge branch 'main' into custom_card_functions@main
chlebowa e2f51bb
improve argument name
chlebowa f4936d6
remove hydrate_function
chlebowa 6fd8f74
make card functions pure
chlebowa 33e8e2c
add formal argument
chlebowa d2dc179
Merge branch 'main' into in_teal.reporter@custom_card_functions@main
chlebowa f78e54d
pass card function as is
chlebowa 5be8df0
Merge branch 'main' into in_teal.reporter@custom_card_functions@main
chlebowa a484850
fix documentation
chlebowa fc11907
Merge branch 'main' into in_teal.reporter@custom_card_functions@main
chlebowa 052265f
Merge branch 'main' into in_teal.reporter@custom_card_functions@main
m7pr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is something we need to rethink (the way how to handle custom reporting)
I think there are two ways to handle this:
srv
to bypass and edit reporter card on the fly. (I guess this is not possible now and require some refactoring)(1) is related with the idea we are currently exploring which is called "preprocessing", where app developer can pass any code/function to the
srv
and "postprocess" existing items in that environment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me the solution you are proposing involves a significant rewrite of the modules as well as the framework. In the long run it is probably superior but I imagine it would take time to properly design and implement.
The solution proposed here is quicker to implement and introduces the feature of custom reporting without changing the framework in a meaningful way. It can be used as a temporary measure. I imagine that if a module returns something (be it a
TealReportCard
object or a list of its possible contents), the way to specify its handling would be to pass a function.I will be happy to discuss further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @chlebowa that this is temporary solution that is quick an easy to apply. We can have a broader discussion on how to improve that in the future, but I think for now this is ready to go and can be extended to other modules (and other packages
teal.modules.clinical
,teal.osprey
etc).