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.

Sign up for GitHub

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

Jump to bottom

qenv - Show R Code check = FALSE notification; moving hash out of teal.slice and teal.data into teal #751

Closed
Tracked by #69
donyunardi opened this issue Oct 7, 2022 · 2 comments · Fixed by #774
Closed
Tracked by #69

qenv - Show R Code check = FALSE notification; moving hash out of teal.slice and teal.data into teal #751

donyunardi opened this issue Oct 7, 2022 · 2 comments · Fixed by #774
Assignees
@mhallal1
Labels
core

Comments

@donyunardi
Copy link
Contributor

donyunardi commented Oct 7, 2022 •
edited by nikolas-burkoff
Loading

From @nikolas-burkoff
Do we care about when check = FALSE having some notification in the SRC like we currently do?

Also we are losing the check on the hash of the datasets
@donyunardi donyunardi mentioned this issue Oct 7, 2022
Closed
21 tasks
@donyunardi
Copy link
Contributor Author

donyunardi commented Oct 14, 2022 •
edited
Loading

We discussed the topic of the purpose of the check method in TealData objects. @kumamiao mentioned that the app will not run anyway if the data preprocessing code that happened before init() call is invalid. Therefore the check method feels like another extra step and may not be necessary.

Also, there are evidence thatcheck method is currently not working accurately:
insightsengineering/teal.data#97

CoreDev needs guidance from PO (@kumamiao @lcd2yyz) to decide whether or not we need the check method.

tagging @pawelru for awareness

@donyunardi donyunardi changed the title Show R Code check = FALSE notification qenv - Show R Code check = FALSE notification Oct 14, 2022
@nikolas-burkoff nikolas-burkoff self-assigned this Nov 1, 2022
This was referenced Nov 1, 2022
Closed
Closed
@nikolas-burkoff nikolas-burkoff removed their assignment Nov 1, 2022
@nikolas-burkoff
Copy link
Contributor

ok so using the following 2 PRs as a guide: insightsengineering/teal.slice#121 #770 we can recreate the missing information BUT we should remove this functionality out of teal.data and teal.slice and only have it here in teal.

This should be done after the merge of teal_refactor into main and we should check that this change doesn't break the old chunks way of doing things as that is only soft deprecated

@nikolas-burkoff nikolas-burkoff changed the title qenv - Show R Code check = FALSE notification qenv - Show R Code check = FALSE notification; moving hash out of teal.slice and teal.data into teal Nov 1, 2022
@mhallal1 mhallal1 self-assigned this Nov 8, 2022
This was referenced Nov 9, 2022
Merged
Merged
Merged
mhallal1 added a commit that referenced this issue Nov 11, 2022
@mhallal1 @github-actions
add hashing (#774)
bbaf8c9 
closes #751 

The dataset are being hashed using `rlang::hash` in:
1) `data_to_datasets` when using `qenv`
2) `get_rcode` when using chunks `chunks`

-`calculate_hashes` function has been created to be used in both places.
-the filtering is not returned in `get_datasets_code` now but rather in
`get_rcode` and `datasets_to_data`.
-messages and warnings calls are now returned in `get_datasets_code`
instead of character strings.

Please test with `chunks` and `qenv` examples from tmg, tmc ...


Also closes #752

Signed-off-by: Mahmoud Hallal <86970066+mhallal1@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Nikolas Burkoff <nikolas.burkoff@roche.com>
mhallal1 added a commit to insightsengineering/teal.data that referenced this issue Nov 11, 2022
@mhallal1 @dependabot-preview
remove hashing (#119)
71bf77c 
related to insightsengineering/teal#751

remove datasets hashing and transfer it to `teal`.

Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
mhallal1 added a commit to insightsengineering/teal.slice that referenced this issue Nov 11, 2022
@mhallal1 @dependabot-preview
@insights-engineering-bot @arkadiuszbeer
remove hashing (#126)
8bf4c29 
related to insightsengineering/teal#751

Remove datasets hashing and transfer it inside `teal`.

Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: insights-engineering-bot <68416928+insights-engineering-bot@users.noreply.github.com>
Co-authored-by: arkadiuszbeer <86738093+arkadiuszbeer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mhallal1 mhallal1

Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add hashing insightsengineering/teal
3 participants
@donyunardi @nikolas-burkoff @mhallal1