augustocdias / vscode-shell-command

A task helper to use system commands as input
MIT License
55 stars 20 forks source link

Take PR#19 Beyond Pre-Set Environment Variables And Take Into Account EnvVars Set In Json #73

Closed vindicatorr closed 10 months ago

vindicatorr commented 10 months ago

https://github.com/augustocdias/vscode-shell-command/pull/19#issuecomment-1902904006

MarcelRobitaille commented 10 months ago

Thanks for opening a dedicated issue. So, you expect tasks.json's options.env object to be available in this plugin's ${env:variable} substitution? I think this is a reasonable assumption.

This is documented as:

The environment of the executed program or shell. If omitted the parent process' environment is used.

One could make the argument that this only applies to tasks started from tasks.json, not inputs (this plugin provides inputs even if they are executable commands like tasks).

Furthermore, this could be a breaking change for some people. If I read this correctly, the process's environment will be only options.env if this is set, it will not be merged with the parent process's environment. Either way, it's a breaking change.

May I ask what is your use case? Is there a possibility to work around this?

vindicatorr commented 10 months ago

I'm creating a list of tasks where the first task would be to initialize non-existent variables that would be re-used for the other tasks rather than calling the extension for every other task.

I use jq to set/update the initialized variables in the .json file. Because this extension doesn't seem to check the existence of the tasks.options.env object, I had resorted to using the config object (my limited workaround). However, the issue with that , as shown in the vscode issue link that was closed (from my comment in that PR), any config value that is referenced but does not exist, will throw an error, rather than leave it up to the referencer to determine what to do with a null value.

So to go with the

If omitted the parent process' environment is used.

statement, I'd think one would check the existence of tasks.options.env and use that if it exists. If it doesn't then continue using the parent process' environment.

MarcelRobitaille commented 10 months ago

Thank you for clarifying.

I'd think one would check the existence of tasks.options.env and use that if it exists. If it doesn't then continue using the parent process' environment.

Exactly. And if someone relies on using environment variables (from ~/.bashrc or whatever) in their inputs, and all of a sudden this variables are not defined and instead only tasks.options.env is set, this would break their setup.

~/.bashrc:

export PATH="/usr/bin/whatever/:$PATH"

.vscode/tasks.json:

{
    "options": {
        "env": {
            "VARIABLE_I_ONLY_WANT_FOR_TASKS_NOT_INPUTS": "blah",
        }
    }
}

They go and run their task which used to work, but now PATH is not set in their inputs, only VARIABLE_I_ONLY_WANT_FOR_TASKS_NOT_INPUTS is.

Alternatively, we could merge the parent process's environment with options.env. However, this is still a breaking change.

~/.bashrc:

export FOO="1"

.vscode/tasks.json:

{
    "options": {
        "env": {
            "FOO": "2",
        }
    }
}

If the user expects the previous behaviour where they could have a separate FOO for inputs and tasks, this would break. This is less likely, but still technically a breaking change.

vindicatorr commented 10 months ago

? If they never set options.env, then they'll still be getting their parent process environment variables. If they set options.env, then they would get those options only (as well as whatever is native to the system via /etc/profile* (just tested)).

Maybe I'm not seeing the issue you're trying to state, when I'm stating the extension should give both possibilities (like vscode does) , leaving it up to the writer to decide which approach to take.

MarcelRobitaille commented 10 months ago

Then it's the second option. Technically a breaking change, but I think the risk is low that it would actually cause problems.

vindicatorr commented 10 months ago

I guess it could technically be called a "breaking" change only in that it would be a change that follows what vscode already does, in which case it was broken to begin with.

But I'd agree with that risk being so low because if they were already using options.env, then they already had the expectation of the kind of environment would be used, even if it wasn't referenced from the input.

EDIT0: And I'll add, that I'd think it'd be better to implement a "breaking change" if it is to correct what was previously broken.

MarcelRobitaille commented 10 months ago

https://xkcd.com/1172/

vindicatorr commented 10 months ago

Tested and confirmed. Thanks.