refactor!(install): use builder pattern #299
Conversation
mrcjkb
force-pushed
the
mj/push-kytzywksurzs
branch
from
1e4e5a1 to
82ededa
Compare
mrcjkb
force-pushed
the
mj/push-vnqrqmtzrxry
branch
from
260a3d8 to
169598d
Compare
mrcjkb
force-pushed
the
mj/push-kytzywksurzs
branch
from
82ededa to
38c940d
Compare
mrcjkb
force-pushed
the
mj/push-kytzywksurzs
branch
from
38c940d to
8452798
Compare
| Self { | ||
| config, | ||
| package_db: None, | ||
| packages: Vec::new(), |
There was a problem hiding this comment.
issue: i think the builder pattern doesn't quite fit in install's case since one can install an empty set of packages by simply running Install::new().install() (which makes little sense). Can we enforce at the type level that at least one package should be provided (without having to do runtime checks in install())?
There was a problem hiding this comment.
This is also possible without the builder pattern, by passing in an empty vec.
We could change the new constructor to take a NonEmpty<PackageReq>.
I'd be in favour of that, but that would be for a separate PR.
There was a problem hiding this comment.
Come to think of it, sometimes we want it to be possible for the list to be empty, for example when installing test dependencies.
| package_db: Option<RemotePackageDB>, | ||
| packages: Vec<(BuildBehaviour, PackageReq)>, | ||
| pin: PinnedState, | ||
| progress: Option<Arc<Progress<MultiProgress>>>, |
There was a problem hiding this comment.
nitpick: I just realized this exists in the other PRs (including the earlier PR that was merged) as well: we don't need to wrap this in an option, since the whole purpose of Progress<T> was to allow us to pass NoProgress :p
There was a problem hiding this comment.
The reason this is wrapped in an Option is so that we can fall back to default behaviour if the caller doesn't pass in one.
(right now, the default behaviour is to create a new MultiProgress).
Stacked on #298