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.

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

Embrace the tibble! #250

Merged
merged 12 commits into from
Feb 10, 2018
Merged

Embrace the tibble! #250

merged 12 commits into from
Feb 10, 2018

Conversation

wlandau
Copy link
Member

@wlandau wlandau commented Feb 10, 2018

Now used for workflow plan data frames and the output of dataframes_graph(). Supports #169 and #247. Remaining challenges:

@wlandau
Copy link
Member Author

wlandau commented Feb 10, 2018

Main benefits:

  • Printing.
  • For workflow plans later on, easier integration with custom future evaluators and commands as expressions/language objects.

@codecov-io
Copy link

codecov-io commented Feb 10, 2018

Codecov Report

Merging #250 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #250   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          64     64           
  Lines        4468   4486   +18     
=====================================
+ Hits         4468   4486   +18
Impacted Files Coverage Δ
R/cache_log.R 100% <100%> (ø) ⬆️
R/sanitize.R 100% <100%> (ø) ⬆️
R/commands.R 100% <100%> (ø) ⬆️
R/dataframes_graph_utils.R 100% <100%> (ø) ⬆️
R/generate.R 100% <100%> (ø) ⬆️
R/build_times.R 100% <100%> (ø) ⬆️
R/dataframes_graph.R 100% <100%> (ø) ⬆️
R/workplan.R 100% <100%> (ø) ⬆️
R/check.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72ef4a9...d589380. Read the comment docs.

@lintr-bot
Copy link

R/commands.R:91:8: style: Place a space before left parenthesis, except in a function call.

for(attribute in c("srcref", "srcfile", "wholeSrcref")){
       ^

@wlandau wlandau merged commit 49903b0 into master Feb 10, 2018
@wlandau wlandau deleted the tibble branch February 10, 2018 03:52
wlandau pushed a commit that referenced this pull request Feb 10, 2018
@krlmlr
Copy link
Collaborator

krlmlr commented Feb 10, 2018

Maybe use hms for durations?

@wlandau
Copy link
Member Author

wlandau commented Feb 11, 2018

I just tried. It might work if I did it differently, but it seems to turn the times into NA's.

load_basic_example()
make(my_plan)
bt <- build_times()
library(lubridate)
bt$elapsed <- hms(bt$elapsed)

## In .parse_hms(..., order = "HMS", quiet = quiet) :
##   Some strings failed to parse, or all strings are NAs

head(bt)

##                     item   type elapsed   user system
## 1 coef_regression1_large target    <NA> 0.004s     0s
## 2 coef_regression1_small target    <NA> 0.004s     0s
## 3 coef_regression2_large target    <NA> 0.008s     0s
## 4 coef_regression2_small target    <NA> 0.004s     0s
## 5             data.frame import    <NA> 0.008s     0s
## 6                   knit import    <NA>  0.06s     0s

@krlmlr
Copy link
Collaborator

krlmlr commented Feb 11, 2018

This works for me:

bt$elapsed %>% lubridate::as.difftime() %>% hms::as.hms()

I wonder if we can change hms so that the first step in the pipe is not required. At this point it seems better to add pillar support to lubridate (or the other way round).

@wlandau
Copy link
Member Author

wlandau commented Feb 11, 2018

It works for me too, but I'm not sure I like the output format:

> bt$elapsed %>% lubridate::as.difftime() %>% hms::as.hms()
## 00:00:00.008
## 00:00:00.004
## 00:00:00.004
## 00:00:00.007
## 00:00:00.008
## 00:00:00.010

@krlmlr
Copy link
Collaborator

krlmlr commented Feb 20, 2018

The dev version of pillar now uses format() for classed numerics, the output should be fine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants