-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add FromIterator<Process> for Launch #401
Conversation
I'm not 💯 sold on the idea that an iterator of I however do agree that it should be easier to construct a launch config when you just have a collection of processes. What do folks think about adding a #[must_use]
pub fn processes<P: Into<Process>, I: IntoIterator<Item = P>>(mut self, processes: I) -> Self {
for process in processes {
self.processes.push(process.into())
}
self
} Using the API above, the #[test]
fn launch_from_iterator() {
let launch: Launch = Launch::new().processes(
["web", "worker"]
.iter()
.map(|&name| ProcessBuilder::new(name.parse().unwrap(), name).build()),
);
assert_eq!(launch.processes[0].command, "web");
} IMO, this is not worse in terms of verbosity than the original API proposal from this PR as this only adds one line, but also removes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment on the PR.
Plus several similar APIs in libcnb, eg |
Great point. This seems fine: let launch: Launch = Launch::new().processes(
["web", "worker"]
.iter()
.map(|&name| ProcessBuilder::new(name.parse().unwrap(), name).build()),
); Though, I've kind of developed a taste for chaining methods over nested arguments. I guess I'm noticing a pattern in my buildpacks that looks like
These just read inside out to me compared to a chained approach, and I guess that was partly motivating the |
Currently, collecting a dynamically created list of
Process
es into aLaunch
has to be done manually. Something like this:With a
FromIterator
, it gets a bit easier: