pharmaverse / admiral

ADaM in R Asset Library
https://pharmaverse.github.io/admiral
Apache License 2.0
220 stars 61 forks source link

Closes #2413 abort_query_data: replace calls to deprecated functions and use cli for messages #2417

Closed bundfussr closed 4 months ago

bundfussr commented 4 months ago

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the main branch until you have checked off each task.

github-actions[bot] commented 4 months ago

Code Coverage

Package Line Rate Health
admiral 97%
Summary 97% (4690 / 4818)
bundfussr commented 4 months ago

I have merged the 2403-cli_abort branch into this one to pass the checks.

zdz2101 commented 4 months ago

I have merged the 2403-cli_abort branch into this one to pass the checks.

Just to be clear this PR essentially contains all the changes made in https://github.com/pharmaverse/admiral/pull/2412 ?

bundfussr commented 4 months ago

Just to be clear this PR essentially contains all the changes made in #2412 ?

Yes

bms63 commented 4 months ago

@bundfussr amazing work!! The snapshpts really help with reviewing the informative/warning/error messages!! I'm wondering if we should push more in that direction for everything. I spotted a few typos and redundant messages.

Also, I started questioning the return of sub-functions in the error messages as they are not explicitly called by the users - perhaps something for us to mull over as it might lead to some confusion.

bms63 commented 4 months ago

@kaz462 @manciniedoardo @sadchla-codes @zdz2101 @millerg23

Hi all. It would be really helpful if you all, at the minimum, can look over the snapshots and see if the error/warnings messages make sense. It was really nice way to review this stuff, i.e. the snapshots. I spotted some typos, missing words and so having more eyes on this would be really helpful. Some of these typos have been around for a while - eeehh!!

Going deeper is also appreciated, but would love to get this merged in by EOD Tuesday.

bms63 commented 4 months ago

Okay! It got weird for a minute, but now we are back on track!! Thank you @bundfussr for this update!!

kaz462 commented 4 months ago

@bms63 @bundfussr I just did a review and left some comments, all minor stuff, maybe I can update with a new PR?

bms63 commented 4 months ago

@bms63 @bundfussr I just did a review and left some comments, all minor stuff, maybe I can update with a new PR?

oh no!! sorry @kaz462 - yes please restore the branch and create a new PR. I don't think the code will show differences anymore, but you can just refer to this one and @bundfussr can update. thanks!!

kaz462 commented 4 months ago

oh no!! sorry @kaz462 - yes please restore the branch and create a new PR. I don't think the code will show differences anymore, but you can just refer to this one and @bundfussr can update. thanks!!

All good! I went ahead and updated here: https://github.com/pharmaverse/admiral/pull/2423