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

Allow user to inherit on a per-variable basis #5

Open
epage opened this issue Oct 13, 2017 · 7 comments
Open

Allow user to inherit on a per-variable basis #5

epage opened this issue Oct 13, 2017 · 7 comments

Comments

@epage
Copy link

epage commented Oct 13, 2017

For motivation, see assert-rs/assert_cli#51

@Freyskeyd
Copy link
Owner

Hello @epage ,

The main question is what the API should look like. As Environment grows different var manipulation functions (inherit, append, etc), should they be var_append, inherit_var, etc or var(key).inherit(), var(key).append(text), etc?

Can you be more specific about var(key).inherit() etc?

let env = Environment::empty()
    .inherit_var("PATH")
    .inherit_vars_matching("CARGO_.*")
    .insert("FOO", "BAR");
let env = Environment::inherit_matching("CARGO_.*")
    .inherit_var("PATH")
    .insert("FOO", "BAR");

@epage
Copy link
Author

epage commented Oct 13, 2017

Just a trade off to make in a fluent API. Do you keep the entire API flat or do you give it some hierarchy to make things easier to browse (in docs or auto-completion)

let env = Environment::empty()
   .inherit_var("PATH")
   .var_append("PATH", ";path")
   .insert("FOO", "BAR");

let env = Environment::inherit()
   .insert("FOO", "BAR");
   .remove("FOO", "BAR");

or

// Environment::var(self) -> Var
// Var::inherit(self) -> Environment
// Var::append(self, value) -> Environment
// Var::remove(self) -> Environment
// Var::insert(self, value) -> Environment

let env = Environment::empty()
   .var("PATH").inherit()
   .var("PATH").append(";path")
   .var("FOO").insert("BAR");

let env = Environment::inherit()
   .var("FOO").insert("BAR");
   .var("FOO").remove("BAR");

The downside to this is if you have other inherit methods (inherit_vars, inherit_matches) they will not longer be next to each other in the docs or in auto-completion.

I took this approach in assert_cli for stdin/stdout. My main motivation was to make it easier to reuse code.
https://github.com/killercup/assert_cli/blob/master/src/assert.rs#L112

@Freyskeyd
Copy link
Owner

I like the fluent API you are describing but I don't understand the append is it an append on the variable's value?

What's about the insert?

@epage
Copy link
Author

epage commented Oct 13, 2017

Note: one downside to the API I described is that rustfmt does not, by default, allow multiple .var("FOO").insert("BAR"), it will be split across lines.

append is it an append on the variable's value?

Yes. The use case is for variables that contain lists, like PATH. It provides a simpler way of extending the variable.

What's about the insert?

var(key).insert(value) was meant to be like .insert(key, value). In this nested API, it might be better to give it a different name. var(key).create(value)? Unsure. In some respects it seems weird to use var(key) to access a non-existent item but that is also how the proposed var(key).inherit() works. shug

@Freyskeyd
Copy link
Owner

Freyskeyd commented Oct 13, 2017

maybe:

  • var(key).equal(value)
  • var(key).set_to(value)

Maybe rustfmt can be configure for that ?

@epage
Copy link
Author

epage commented Oct 13, 2017

equal sounds like a test to me.

assign would be more semantically clear and I kind of like.

set_to I have this weird thing about avoiding prepositions like that. shrug

@Freyskeyd
Copy link
Owner

I start implementing it in #9

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

No branches or pull requests

2 participants