Skip to content
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

Explore ways to make the LayerEnv API more consise #73

Closed
edmorley opened this issue Sep 8, 2021 · 3 comments
Closed

Explore ways to make the LayerEnv API more consise #73

edmorley opened this issue Sep 8, 2021 · 3 comments
Labels

Comments

@edmorley
Copy link
Member

edmorley commented Sep 8, 2021

I've split this review comment out of #30 so it doesn't get lost, since the PR has already been merged.

Originally posted by @schneems in https://github.com/Malax/libcnb.rs/pull/30#discussion_r698781818


As I'm looking at this code, I'm curious if there's a more concise way to represent this logic:

layer_env.insert(TargetLifecycle::All, ModificationBehavior::Append, "VAR", "bar");

For example (without implementation constraints) could be:

layer_env.all.append("VAR", "bar");
layer_env.build.append("VAR", "bar");
layer_env.launch.append("VAR", "bar");

Or something like:

layer_env.append_all("VAR", "bar");
layer_env.append_build("VAR", "bar");
layer_env.append_launch("VAR", "bar");

What you've got now is great to move forward. As I'm trying to figure out what makes a "good" interface in Rust, I would be interested to hear your thoughts (Again, non-blocking...we can take it offline to chat).

@edmorley
Copy link
Member Author

edmorley commented Sep 8, 2021

Also another related comment from that PR:

Originally posted by @schneems in https://github.com/Malax/libcnb.rs/pull/30#discussion_r698790544


I was confused by this TargetLifecycle::Build here. I couldn't figure out why TargetLifecycle::All was being specified above, but just build here. If I'm understanding correctly it's because we're currently in a build environment so we only want to apply modifications to build to the current env.

That makes sense as I type it out, but it was unclear on first pass. Right now I'm trying to think of cases where we would want to assign to the current env but have it not be build. I'm not sure I can come up with any.

I'm also left somewhat wanting to compress/combine LayerEnv and Env updates. It's a two step process but seems like I always want to do them in lockstep. I would need to see it utilized in a buildpack, but I bet that I'll insert the env var and then forget to update the current env.

Maybe some interface like this would help prevent that situation:

layer_env.insert_and_apply(TargetLifecycle::All, ModificationBehavior::Default, "VAR2", "default", &env);

@edmorley edmorley changed the title Explore ways to make the LayerEnv API more consise Explore ways to make the LayerEnv API more consise Sep 8, 2021
@edmorley
Copy link
Member Author

Closing as resolved, given the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant