-
Notifications
You must be signed in to change notification settings - Fork 20
Some updates to the Crossbow report #92
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
Conversation
Some actual arrow_build_table tests, a CI job r 4.4 to 4.5 more imports, more test More test Attempt to use PPPM for binaries More minimal CI job? Also need working directory for renv? Do actually need system deps update renv Call it cran? is PPM actually the magic?
library(tibble) | ||
library(dplyr) | ||
library(lubridate) | ||
library(glue) | ||
library(tidyr) |
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.
We get these already in the quarto doc, and this does smell a little funny in a script, but makes testing much easier so I think isn't so bad. Alternatively we could us ::
for all of the functions used by these in this cript.
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.
Fine by me. Doesn't smell too funny to me :)
Oh awesome, thanks! Will review tomorrow! |
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.
A couple small comments. Thank you for this, especially the tests.
library(tibble) | ||
library(dplyr) | ||
library(lubridate) | ||
library(glue) | ||
library(tidyr) |
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.
Fine by me. Doesn't smell too funny to me :)
df$arrow_commit[df$fail_label == label] | ||
} | ||
|
||
arrow_build_table <- function(nightly_data, type, task, to_day = today()) { |
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.
When I saw this I thought the to_day
was actually in the form of to_*
thinking that it did something like that. What about something like current_day
?
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.
Heh I was being a little cheeky, but yeah readable is more important :P
filter(fails_plus_one <= 9 | grepl("failure|build", fail_label)) | ||
|
||
## inner_join to ordered data | ||
# Join the failure timeline labels with the actual build data |
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.
Thank you for these comments ❤️
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.
Mostly thank Claude :P But in truth I had Claude comment and then went and heavily edited.
Co-authored-by: Sam Albers <[email protected]>
Co-authored-by: Sam Albers <[email protected]>
Thanks for this @jonkeane ! |
It's probably easiest to review by commit, or at least from after the first commit (which is lots of diff, but that's just letting air control the formatting).
A few notable changes:
cc @boshek @amoeba @assignUser