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

Proposal: Modify and retrieve the local environment variables #120

Open
joshgoebel opened this issue Sep 11, 2021 · 7 comments
Open

Proposal: Modify and retrieve the local environment variables #120

joshgoebel opened this issue Sep 11, 2021 · 7 comments

Comments

@joshgoebel
Copy link
Contributor

I may need this soon and I wanted to get thoughts/feedback on the API. We'd likely need to wrap:

  • uv_os_environ
  • uv_os_unsetenv
  • uv_os_setenv
  • uv_os_getenv

Perhaps:

Process.env.all // ?
Process.env.toMap // ?
Process.env["PATH"] 
Process.env["PATH"] = "/usr/bin/"
Process.env.get("PATH")
Process.env.set("PATH", "/usr/bin/")
Process.env.unset("PATH")

To me the [] and get, set, unset seem reasonable it's the all and toMap I'm wondering if anyone has any better ideas for accessing the entire environment at once. I also thought Process.env seemed nice, but one could also imagine Process.environment or even Environment on it's own I suppose.

Related: joshgoebel#11

@PureFox48
Copy link

I think myself a new Env class in the os module might be better than putting more stuff in the Process class.

As far as methods are concerned, I'd just mimic most of what we have in the Map class except , of course, they'd all be static. This would be more familiar to folks than coming up with new names.

So we'd have:-

  • a static indexer to get and set particular environment variables.
  • a containsKey method to return whether a particular variable exists.
  • a count property to return the number of such variables.
  • a remove method to delete a variable and its associated value
  • a keys property to return a sequence of all variables to iterate through.
  • an entries property, say, to return a sequence of key/value map entries to iterate through at the same time.

I'd omit clear (too dangerous!) and values (probably unnecessary)

Some examples to see how this would look:

var path = Env["PATH"]
Env["PATH"] = "/usr/bin/"
var c = Env.count
Env.remove("WHATEVER")

for (key in Env.keys) System.print("%(key) = %(Env[key])")

for (me in Env.entries) System.print("%(me.key) = %(me.value)")

@glennj
Copy link

glennj commented Sep 22, 2021

Although the environment is a property of a process, from an interface perspective, a separate Env module is nicer.

For me, the environment is a collection of NAME=value pairs. Calling them "keys" doesn't sound right.

Env.names   // unordered list of variable names

If Env can have Sequence as a superclass, like Map, then you get all the iteration mostly for free

class EnvEntry {
  construct new(name, value) {
    _name = name
    _value = value
  }

  name { _name }
  value { _value }

  toString { "%(_name)=%(_value)" }
}

@joshgoebel
Copy link
Contributor Author

If Env can have Sequence as a superclass, like Map, then you get all the iteration mostly for free

I was worried since the underlying data comes from the OS via syscall... so that's why I went with toMap originally vs making it iterable - since we have nothing to iterate over unless we cache the full env inside Wren. So if someone wants to iterate they just request a map (which is iterable)... that was my thinking at least.

Calling them "keys" doesn't sound right.

Yes, we should prefer simple, not easy. If the domain is different then the naming should be natural to the new domain. IE, not every "hash-like" object should have a Map interface verbatim.

@glennj
Copy link

glennj commented Sep 22, 2021

Still needs some native functional methods:

Env.each {|variable| System.print("%(variable.name}=%(variable.value)")}
Env.where {|var| var.name.endsWith("PATH")}

And that EnvEntry class should clearly be named EnvVariable

@joshgoebel
Copy link
Contributor Author

joshgoebel commented Sep 22, 2021

I'd say those would have to be deferred to Map, since again those things depend on the itterable protocol, and we do not have that data to iterate over - it could be changing out from under us, etc...

We could write naive ones, but to be a good Wren citizen they should be a Sequence, for chain ability, etc...

@joshgoebel
Copy link
Contributor Author

joshgoebel commented Sep 22, 2021

I should also say with many things like this I'm in favor of "minimal viable functionality" vs "get it 100% the first pass"... if we only supported some basic functionality in a first pass and then went back later and added more, that'd be great. Of course we can still discuss the full scope here, but I wouldn't prefer to limit an initial implementation to 100% if say 80% was super useful to have. To me iterability is one of those "nice to have", but not "required" items. (doubly so if you can get it via toMap if you really need it)

@glennj
Copy link

glennj commented Sep 22, 2021

I hear you. toMap makes sense. Perhaps also (or instead of) toList returns a list of EnvVariable objects

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

No branches or pull requests

3 participants