Skip to content

Conversation

@machow
Copy link
Contributor

@machow machow commented Jul 5, 2022

Quick note on pre-commit:

  • I think some of the current hadn't been run through pre-commit, so were tweaked a bit by black.
  • I set flake8 to allow lines to be 90 characters so it wouldn't error for somethings black won't format (docstrings etc..). We can remove and re-format them by hand if needed!

TODO:

  • : If people save metrics as a CSV, then we need to parse their date index as a datetime. (I set everything to use type="arrow" as a placeholder for now, so tests would pass. But we should change before merging).
  • : Need to test different overwrite conditions (e.g. when it should error out, check that it works as expected).

@machow
Copy link
Contributor Author

machow commented Jul 6, 2022

See discussion in issue above -- we need to make sure we sync up strategy for specifying pin types with vetiver-r!

@isabelizimm
Copy link
Contributor

isabelizimm commented Jul 6, 2022

Commented in issue, but putting over here for record-keeping--I think deciding pin type based on the original pin is the right way to go :D Looks like the R side will have dots to match this

@machow
Copy link
Contributor Author

machow commented Jul 12, 2022

@isabelizimm I think this is ready--feel free to add commits + merge, or if there's anything to change I'm happy to do it!

Copy link
Contributor

@isabelizimm isabelizimm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! arrow handling can come later since I currently do not have it implemented anywhere else in vetiver 🚢

@isabelizimm isabelizimm merged commit 1addc8c into rstudio:main Jul 12, 2022
@machow
Copy link
Contributor Author

machow commented Jul 13, 2022

Ah yeeahhh! This should handle arrow (there aren't arrow tests here, since it's not a CI dep, but there are arrow tests in pins)

@isabelizimm isabelizimm mentioned this pull request Jul 21, 2022
3 tasks
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.

2 participants