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

Change pRojects to allow the use of the checkpoint package #32

Merged
merged 10 commits into from
Oct 24, 2018

Conversation

stephlocke
Copy link
Member

When I first setup pRojects I put in a boolean flag for the use of packrat. Since then I'd been considering allowing checkpoint, especially as packrat can really get on my nerves!

This change amended the function inputs to have packagedeps not packrat as an argument and amended some code.

To avoid changing expected behaviours, the functions still use packrat by default.

@stephlocke
Copy link
Member Author

Hey @jonmcalder, what do you think of this change? Do you think anything else needs doing on it?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 49.198% when pulling 91cd32c on feature-checkpoint into 91ed1de on master.

@jonmcalder
Copy link
Contributor

I approve! packrat is a rather heavy dependency and can be quite frustrating especially for newer useRs. I haven't actually tried checkpoint before but this change looks great.

I'll review in more detail this evening when I've got time.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 49.198% when pulling 167a16c on feature-checkpoint into 91ed1de on master.

Copy link
Contributor

@jonmcalder jonmcalder left a comment

Choose a reason for hiding this comment

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

createTrainingProject() needs to be updated, since it still includes:

# Installation dir
  usePackrat <- !methods::hasArg("packrat")
  if (!usePackrat)
    usePackrat <- list(...)[["packrat"]]
  installDir <- ifelse(usePackrat, name, .libPaths())

and then further on:

if (usePackrat) {
    packrat::snapshot(name)
    packrat::restore(name)
  }

This results in an error when trying to create a training project with the new gadget/template.

Position: right
Parameter: packagedeps
Widget: SelectInput
Label: Which reproducibility package fields to use
Copy link
Contributor

@jonmcalder jonmcalder Sep 22, 2017

Choose a reason for hiding this comment

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

I would:

a) shorten this label (e.g. just Reproducibility), since a long line messes up the alignment in the template UI
b) group it with the other dropdowns by positioning the check boxes to the right
c) the same applies (both label and alignment) to all the other templates

Before:
image

After:
image

Still looks pretty terrible, but slightly less terrible 😄

This also improves additional package handling and removing a package dep 🎉
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.03%) to 46.875% when pulling 86163bf on feature-checkpoint into 91ed1de on master.

@stephlocke
Copy link
Member Author

Thanks for the feedback!

Note in 1cf5ece I also improved arg (imo anyway ;) ) handling ...

  • this removed methods as a dependency
  • used an exported function from devtools instead of an internal function
  • stops if slide/handout engines requested are not currently installed

@maelle
Copy link
Contributor

maelle commented Aug 30, 2018

@stephlocke silly question, should checkpoints be the default?

@stephlocke
Copy link
Member Author

Sure, I think we can make "breaking" type changes like this, esp as this only does package setup

Suggested packages aren't always installed so this prevents the reproducibility oriented package availability checks from triggering
@stephlocke stephlocke merged commit 11fb837 into master Oct 24, 2018
@stephlocke
Copy link
Member Author

@maelle this was getting to the same error on master but I think the checkpoint code could help you fix things so I've now merged it in

@maelle
Copy link
Contributor

maelle commented Oct 24, 2018

Ok, will look next week probably. I think that #60 and also letting the function clean and say what was wrong will help, because clearly something went wrong inside createTrainingProject and the current code doesn't let me easily what.

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.

4 participants