-
Notifications
You must be signed in to change notification settings - Fork 6
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 ability to query modify process environment #11
Comments
I feel that this would be better to be on its own class inside the import "os" for Env class EnvBridge {
static foreign f_environ
static foreign f_setenv(key, value)
static foreign f_unsetenv(key)
static foreign f_getenv(key)
}
class EnvValue {
construct new(key, value) {
_key = key
_value = value
}
key { _key}
value {_value}
value(val) = {
EnvBridge.f_setenv(key, val)
_value = val
}
}
class EnvBag {
construct new(items) {
_items = {}
for(item in items) {
_items[item.key] = item
}
}
items { _items}
}
class Env {
static all { EnvBag.new(EnvBridge.f_environ.map { |item| EnvValue.new(item[0], item[1]) }) }
static get(key) {
var item = EnvBridge.f_getenv(key)
EnvValue.new(item[0], item[1])
}
static get(key, default) { get(key) || default }
static set(key, value) { EnvBridge.f_setenv(key, value) }
static remove(key) { EnvBridge.f_unsetenv(key) }
static load(content) {
// parses and loads a dot env string
}
} |
Overall looks nice.
This can't really be so simple though. Likely the raw C function will have to return a Wren List (since there aren't good Map functions on the C side) and then we convert it to a map on the Wren side...
Well, environment is something a process has technically (right?)... still think |
Technically not necessary, is just to avoid mixing foreign internal classes with public userland ones.
class Process {
static env {Env}
} |
I thought that's why we have the |
Some Usage Examples import "os" for Env
// import "os" for Process
// will throw a Fiber.abort if the env is not found
// fetch(key, error)
Env.fetch("USER", "$USER is not set")
// will return clsource if USER is not set
Env.get("USER", "clsource")
// will return null if USER is not set
Env.get("USER")
// will return clsource if the USER is not set. Also will set the value to clsource if not previously set
Env.getOrSet("USER", "clsource")
// will set the value for the key in the Env
Env.set(key, value)
// will delete the key from the env
Env.remove(key)
// Will return all the environment values as a Map
Env.all
// Will return all the environment values as a EnvValue map
Env.values |
|
EnvValue is a class that would help you do this // will set the env USER to clsource
Env.values["USER"] = "clsource" By using
get would return the raw value. If you want the EnvValue you would use the |
Why not:
|
Indeed class Env {
static [key] = (value) {
System.print("%(key): %(value)")
}
}
Env["USER"] = "clsource" |
Yeah I imagined we'd have |
There is this argument https://www.doppler.com/blog/goodbye-env-files that says is better to use json as a storage of env vars instead of dotenv files because json is more standarized. Maybe a function can be added. load(map) That will load all envs from a map. This map can be created using an external parser |
Of all of this I care the least about the dotenv stuff because to me that feels like it's own thing... really it's just File.load and a loop on top of either JSON or String.split... but at the same time for that reason it also doesn't feel like much scope creep either. So shrugs. I'd honestly rather support the "standard" though than scope creep into other "good ideas"... if someone wants to load the ENV from JSON vs and env file, that's trivially easy to do on their own. |
I mean is not necessary to include parsing dotenv files. Just include a way to pass a map and then it will load to env. |
Ah, that sounds a bit more reasonable. :) |
Well, before we can do anything useful with the environment we need ENV capabilities in the CLI, which currently do not exist, we likely need to wrap:
Perhaps:
Thats likely first a todo/discussion for Wren CLI and Wren Console, as I think environment should be part of the CLI core library...
Originally posted by @joshgoebel in joshgoebel/wren-essentials#7 (comment)
The text was updated successfully, but these errors were encountered: