Function deletePackage added

[tonytonov]

Following up the discussion in #81, created a deletePackage functionality. Documentation and tests are included. Also spotted two minor inconsistencies in the existing code -- see diff for details.

The tests don't cover mac.binary -- I am using a snapshot which doesn't have Mac packages for 3.5 for some reason -- maybe there's a simple workaround you can come up with. A test case works with a dependency graph of existing packages -- let me know if you need details. There are some explanatory comments as well.

I ran devtools::check, and apart from the vignette building (which seems like a process performed differently?), everything looks good.

[codecov-io]

Codecov Report

Merging #119 into master will increase coverage by 0.75%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   74.05%   74.81%   +0.75%     
==========================================
  Files          13       14       +1     
  Lines         636      663      +27     
==========================================
+ Hits          471      496      +25     
- Misses        165      167       +2

Impacted Files	Coverage Δ
R/addPackages.R	81.53% <100%> (-0.15%)	⬇️
R/deletePackages.R	92.85% <92.85%> (ø)

---

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 abe1b31...d55dd8f. Read the comment docs.

[andrie]

Thanks for this work.

I have created a new branch 119-deletePackages that rebases your PR onto the current dev branch, and I suggest we use that branch as the working copy.

[andrie]

I have made some comments in the code, but I have one remaining, bigger question.

If I understand this PR correctly, when you remove a package, all of the reverse dependencies will also be removed. If that's the case, is there a danger that the repo can end up in an inconsistent state?

For example:

• Package A depends on C
• Package B also depends on C

Now you decide to remove package B. What happens to C, and what does it mean for the state of A?

[tonytonov]

Thanks. I'll go through your comments this week.
I'll have to take a closer look at my tests to confirm that, but I remember I was aware of that problem. So for every package in the "to be deleted" package list I'm checking if this package is safe to delete. The package is safe to delete only when it's not used by any other package.
I'll double-check that and get back to you. Naturally, we may consider introducing stricter rules for deletion.

[tonytonov]

@andrie You mentioned comments in the code -- how can I see them?

[andrie]

Interesting - my expectation was that you would receive an email pointing to the comments. You can access these on github by clicking on the "Files changed" button in the PR. This is the link:

https://github.com/andrie/miniCRAN/pull/119/files

[tonytonov]

Yeah, I looked at that URL. There are supposed to be your comments, right? I can’t see them, maybe there’s something with permissions…? I was able to create a comment there: https://github.com/andrie/miniCRAN/pull/119/files#r300357176, but I can’t see yours.

[andrie]

Hmm. I just added you to "assignees" - hopefully that makes a difference?

[tonytonov]

Sadly, no. I can see the on/off switch for comments, and it is on. Maybe I need to reopen the PR after its' rebase, no clue.

[achubaty]

@andrie I can't see comments either. If you began commenting using the review functionality, you may not have clicked "submit review".

[andrie]

This is a bit unclear to me what's the desired behaviour. It seems that if reverse is in the dots, you always set reverse = FALSE?

I think this will benefit from writing the ... explicitly as arguments.

[andrie]

Thanks for the pointer, @achubaty. I think that was indeed the problem, so hopefully you can now see my comments.

[bollard]

Is there any prospect of this being merged and released? Would be a really useful feature. Thanks

[tonytonov]

Oh my, I never got that last notification about the review, so I didn't know it got stuck on my end. @andrie would you be willing to consider this PR again? If so, I'll try to resurrect it :)