Guard against upgrades in 'pt/maybe-unpack-on-delete'. Fixes #7

[rdallasgray]

Mathrick, I have some time to look at this tomorrow. Do you mind if I merge your PR into a dev branch, add a test and clean it up a bit, then merge it into master?

[mathrick]

Ah, I was actually mostly done with adding the tests, just got a bit distracted. I should be able to push it today still; really doing the tests properly was the only hitch, but I think I'll have it done soon.

On Wed, Sep 18, 2013 at 7:32 PM, Robert Dallas Gray < notifications@github.com> wrote:

Mathrick, I have some time to look at this tomorrow. Do you mind if I merge your PR into a dev branch, add a test and clean it up a bit, then merge it into master?

— Reply to this email directly or view it on GitHubhttps://github.com/rdallasgray/pallet/pull/9#issuecomment-24683629 .

[rdallasgray]

Ah, no problem then. I don't often have free time, so thought I should offer :).

[mathrick]

OK, done. I left the name pt/maybe-unpack-on-delete as is, because it accurately captures the nature of the operation. I have split out the "is it still installed?" tests into a separate function however. Could you take a look at it and tell me whether the code and tests are correct and to your liking?

[rdallasgray]

Merged, thanks!

[rdallasgray]

Mathrick, there were in fact some problems with this which manifested in manual testing (to wit, package.el doesn't recreate the package-alist following a delete, so it will ALWAYS tell you a package is installed immediately following deletion!). I made some modifications to the code accordingly. I also altered pt/installed-p to expect a string -- I'd be interested to hear why you thought it might need to also accept a symbol or other descriptor.

[mathrick]

Ah, I didn't think of testing that, so thanks for catching it. I'm thinking the whole testing harness might need a bit of refactoring to help with the enormous code duplication and to catch problems like this one, where the tests' and package.el's behaviour differs because each test invents its own ad-hoc assumptions.

As for the designator, I did it that way, because the first thing I discovered when I set out to fix it was that package.el used symbols in the alist whilst pallet operated on strings. And since I didn't know the exact codepaths things could take, I played it safe and decided to take any designator to avoid type errors down the road. It's also the usual Common Lisp style, where coercing things to strings is not quite as ridiculously hard and consequently the code to do so would be simple (intern (string designator))

[rdallasgray]

Yeah, I agree somewhat as regards the tests, although if package.el weren't mocked things could get a bit complex. I had a brief chat with Rejeep about creating a public API for Cask, so I could just code against that and trust the interface. He's done a lot of work to encapsulate the hairiness of package.el. I'll try to pick up that conversation.

And thanks for your thoughts re the designator. I had to check, myself, what the type of package-name would be as passed to pt/installed-p. I think it's reasonable to just expect a string, in this case, and avoid unnecessary extra code.

[mathrick]

Oh, I totally think it should be mocked where appropriate. I was just suggesting formalising the whole harness somewhat and gathering it all into one place, so that each test doesn't need a bunch of flets with no clear semantics and no guarantee they correspond in any way to package.el's actual behaviour. Though there's also something to be said for letting it operate on a temporary Cask file. It'd require some extra hair to make sure it's only ever run in batch mode (as it's not safe otherwise without mocking), but in exchange there'd be the knowledge it will test the real package.el.

[rdallasgray]

Yes. All interesting points. I'll need to have a think about how to best do it. I certainly agree as regards cleaning up the test harness, at least.