Gilead-BioStats / gsm

Good Statistical Monitoring R Package
https://gilead-biostats.github.io/gsm/
Apache License 2.0
36 stars 9 forks source link

QC: Eliminate use of cli_alert_*() #1678

Open jonthegeek opened 1 month ago

jonthegeek commented 1 month ago

QC Details

I finally found confirmation that cli_alert_*() functions are leftovers from an older way of doing things in {cli}. Per that ticket:

The main issue I've found is that cli_alert_warning() doesn't actually count as a warning for testing; it's just a message dressed up as a warning. Not a HUGE issue but can make things confusing.

Additional Comments

jonthegeek commented 1 month ago

Hmm, I'm not sure this is quite right. BUT we should be cautious about cli_alert_*() usage.

I think we can gather intent by whether something returns, whether that thing is the expected return, and the tone of the message, but we should check one by one.

jwildfire commented 1 month ago

Makes sense to me to tighten this up. Let's address in v2.1 or v2.2. We may also want to do a more through review of our logging as we move more runs in to GHA.

lauramaxwell commented 2 days ago

Based on discussion in #1814 , we should replace cli_alert_danger() with cli_abort() in most cases so that we don't continue in a pipeline that has failed validation-type checks.