Some updates to the add-jcrinstall-simulator branch

adamcin / oakpal

OakPAL: Oak Package Acceptance Library

http://adamcin.net/oakpal

Apache License 2.0

11 stars 7 forks source link

Some updates to the add-jcrinstall-simulator branch #58

Closed justinedelson closed 4 years ago

justinedelson commented 4 years ago

@adamcin trying to revive this 😄

Merged (with fixing some conflicts) master
Bumped versions
Removed the subPackageId field from SubpackageInstallable -- after doing some experimentation, this seemed to be unnecessary and added overhead.

justinedelson commented 4 years ago

I've done some revisions in 531b39f to the API contract of JcrInstallWatcher to reduce the API surface based on some more experimentation. My key observations:

There's no reason for the dequeueInstallable method -- the intention is to provide an iterable object, so JcrInstallWatcher should just extend Iterable
The variance between openSubpackageInstallable and openRepoInitInstallable is confusing -- these really can be the same method using generics. If a given PathInstallable should only return a single entity (i.e. a JcrPackage), that's OK -- in this context Optional and Iterable provide the same semantics (well... see next bullet)
The one minor awkward thing (at least IMO) is that it might make more sense for the open method in JcrInstallWatcher to return an Iterator instead of an Iterable. This would make some code in OakMachine a bit uglier, but not terribly so.

justinedelson commented 4 years ago

FWIW, the net result of the changes in 531b39f (at least in my experimentation) resulted in about a 40% reduction in the lines of code needed to implement a JcrInstallWatcher

adamcin commented 4 years ago

This is awesome, @justinedelson . Thanks for diving in. On first glance, I agree with many of your comments about naming and confusion, because I was still in stream-of-consciousness mode when I pushed it. I'll take a closer look at your PR when I get a chance tomorrow.

justinedelson commented 4 years ago

thanks @adamcin. If it helps review, I can also create a new branch rebased against master -- right now this PR is against your WIP branch and, since I did the merge from master, it's got some changes here that are unrelated to the actual change.

adamcin commented 4 years ago

@justinedelson so, a little SNAFU on my end. Turns out I had been force pushing this branch, and had a HUGE amount of changes in my local branch. So I relocated that branch to add-jcrinstall-simulator-type-not-pushed-before-jedelson and merged your PR here. The vast majority of overengineering changes probably exist in the other branch, so we'll continue on this branch and copy-paste stuff from the other one if there are valuable things.