Add option nounset to default to strict parsing. #17

sayanarijit · 2020-07-28T06:30:50Z

nounset=True will behave like bash's set -u or set -o nounset
i.e. when any variable is undefined, it will raise UnboundVariable
exception.

This is useful where using the ${VAR:?} syntax is not an option.

codecov-commenter · 2020-07-28T06:32:41Z

Codecov Report

Merging #17 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #17   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          196       208   +12     
=========================================
+ Hits           196       208   +12

Impacted Files	Coverage Δ
expandvars.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf2d20b...2ebaf8e. Read the comment docs.

mnieber · 2020-07-28T06:40:16Z

README.md

+If you want to temporarily disable strict parsing both for `nounset=True` and the `${VAR:?}` syntax, set environment variable `RECOVER_NULL=somevalue`.
+
+```bash
+export RECOVER_NULL=foo


Doing this with an environment variable seems very dangerous. If you have a pipeline that involves your own tool but also someone else's tool that happens to use expandvars, then this other tool can start behaving wrong.

Hi. Valid point too. We shouldn't use export. But this feature comes in handy so often that I'd still keep it.
I have added WARNINGs to remind the consequences of using export.

The problem is that some of the people who would like to see the warning won't be able to see it. If Alice and Bob each create a tool, and I put them both in my stack, then Bob's tool can break when Alice's tool has a script that says "RECOVER_ALL=foo". I'm just an unsuspecting user who would be bitten by this (I probably don't even know that the tools I am relying on use expandvars internally).

Sorry I misspelled it in the first PR (corrected it now). The name is actually EXPANDVARS_RECOVER_NULL. I doubt someone will use this name unintentionally.

`nounset=True` will behave like bash's `set -u` or `set -o nounset` i.e. when any variable is undefined, it will raise `UnboundVariable` exception. This is useful where using the `${VAR:?}` syntax is not an option.

mnieber · 2020-07-28T07:28:34Z

I think you misunderstood my point. If you have a toolchain where 3 different tools are using expandvars internally, then the toolchain will only work correctly if these 3 tools have the same expectation about EXPANDVARS_RECOVER_NULL. For example, if these 3 tools don't do anything with EXPANDVARS_RECOVER_NULL but you add a fourth tool that includes a script (sourced when the toolchain is brought up) which intentionally sets EXPANDVARS_RECOVER_NULL then it will break the other 3 tools.

…

On Tue, Jul 28, 2020 at 9:23 AM Arijit Basu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In README.md <#17 (comment)>: > + +```python +# All the variables must be defined. +expandvars("$VAR1:${VAR2}:$VAR3", nounset=True) + +# Raises UnboundVariable error. +``` + +> NOTE: Another way is to use the `${VAR?}` or `${VAR:?}` syntax. See the examples in tests. + +### export RECOVER_NULL="foo" + +If you want to temporarily disable strict parsing both for `nounset=True` and the `${VAR:?}` syntax, set environment variable `RECOVER_NULL=somevalue`. + +```bash +export RECOVER_NULL=foo Sorry I misspelled it in the first PR (corrected it now). The name is actually EXPANDVARS_RECOVER_NULL. I doubt someone will use this name unintentionally. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGH5TCPHIUCKIQHTEV6TSLR5Z4GNANCNFSM4PKFTVDA> .

sayanarijit · 2020-07-28T07:46:39Z

In that case, I think it will be a fault on the tool developer side. Just like it's a common knowledge that we shouldn't update PATH and other critical environment variables in a script without knowing what we're doing, it's the same here. This variable must be used while running the whole toolchain. Not individual tools. For individual tools, if bypassing strictness is needed and using this variable seems to be the only way, that tool should set the variable in a temporary manner. Without exporting it. Just like we'd temporarily update PATH.

sayanarijit · 2020-07-28T07:50:20Z

If you think I misunderstood again, could you please give a real world example? Or a real world scenario? It'll be easier that way.

mnieber · 2020-07-28T07:54:35Z

Sure! If I'm developing the tool chain and I am completely unaware of expandsvars (don't even know that such a package exists) and I use a bunch of 3rd party tools, then I won't know which ones are using expandvars. I might add a new tool that sets EXPANDVARS_RECOVER_NULL and breaks some of the other tools that also use expandvars. All I see as a developer of the toolchain is that things mysteriously started to break just by adding a tool.

…

On Tue, Jul 28, 2020 at 9:50 AM Arijit Basu ***@***.***> wrote: If you think I misunderstood again, could you please give a real world example? Or a real world scenario? It'll be easier that way. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGH5TGXLTU7AAK6TSKLGHDR5Z7MXANCNFSM4PKFTVDA> .

sayanarijit · 2020-07-28T08:02:42Z

I might add a new tool that sets EXPANDVARS_RECOVER_NULL

If that toos sets the variable locally, it shouldn't affect the other tools. If it exports the variable globally, that tool might also be exporting critical variables like PATH and USER globally. You probably should avoid using anything that depends on this tool.

mnieber · 2020-07-28T08:14:00Z

| You probably should avoid using anything that depends on this tool.

How can I find out what depends on this tool?

sayanarijit · 2020-07-28T08:32:20Z

If adding a tool breaks my system, it means it's "incompatible" with my system. Then I either find a compatible alternative, or investigate why is it incompatible. If after using a tool I see it's messing with my system global variables, I'd be uncomfortable using that tool. On the other hand, it's that tool has to set global environment variable like tmux sets "TMUX" and "TMUX_PANE" and python depends on "PYTHON_PATH" and "PYTHON_HOME", there's a best practice to prefix the variable name properly like "EXPANDVARS_*". If it still sets "EXPANDVARS_RECOVER_NULL" for some other purpose, then it's just an incompatibility issue I don't see ever happening.

And if is sets "EXPANDVARS_RECOVER_NULL" intentionally to influence other tools, Just like direnv and nix does, it should know what it's doing. It's the responsibility of that tool's developer to make it compatible with other tools.

mnieber · 2020-07-28T08:44:08Z

You said it yourself (not me!): You probably should avoid using anything that depends on this tool.
Then I asked "How can I find out what depends on this tool?" and (as you realize) this question is impossible to answer.
Sorry, I will leave it here. I do like the package though, very useful.

sayanarijit · 2020-07-28T08:49:31Z

Thanks 😄 ...

Sorry I can't give a better answer than that. This little feature adds a lot of convenience at the cost an incompatibility issue that's very unlikely to happen. Hence, I'm fine with the trade-off.

mnieber · 2020-07-28T09:48:29Z

Okay, if that's a risk you are willing to take (I think if people get bitten by this, it could be quite bad)

…

On Tue, Jul 28, 2020 at 10:49 AM Arijit Basu ***@***.***> wrote: Thanks 😄 ... Sorry I can't give a better answer than that. This little feature adds a lot of convenience at the cost an incompatibility issue that's very unlikely to happen. Hence, I'm fine with the trade-off. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGH5TCK4ZFF7BMFPQ5A7HLR52GKVANCNFSM4PKFTVDA> .

sayanarijit · 2020-07-28T10:39:45Z

Alright. I'll create a separate issue to see what other users of this library feel about this.

sayanarijit force-pushed the add/feature-nounset branch from 1bb7d12 to 8d886b9 Compare July 28, 2020 06:32

sayanarijit mentioned this pull request Jul 28, 2020

Feature request: support raising an exception on missing env-vars #15

Closed

sayanarijit force-pushed the add/feature-nounset branch from 8d886b9 to 8bba0ab Compare July 28, 2020 06:35

mnieber suggested changes Jul 28, 2020

View reviewed changes

sayanarijit force-pushed the add/feature-nounset branch 3 times, most recently from 3eea4e2 to 65547a5 Compare July 28, 2020 07:02

Add option nounset to default to strict parsing.

2ebaf8e

`nounset=True` will behave like bash's `set -u` or `set -o nounset` i.e. when any variable is undefined, it will raise `UnboundVariable` exception. This is useful where using the `${VAR:?}` syntax is not an option.

sayanarijit force-pushed the add/feature-nounset branch from 65547a5 to 2ebaf8e Compare July 28, 2020 07:08

sayanarijit merged commit 15125a9 into master Jul 28, 2020

sayanarijit mentioned this pull request Jul 28, 2020

About EXPANDVARS_RECOVER_NULL #19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option nounset to default to strict parsing. #17

Add option nounset to default to strict parsing. #17

sayanarijit commented Jul 28, 2020

codecov-commenter commented Jul 28, 2020 •

edited

mnieber Jul 28, 2020

sayanarijit Jul 28, 2020

mnieber Jul 28, 2020

sayanarijit Jul 28, 2020

mnieber commented Jul 28, 2020 via email

sayanarijit commented Jul 28, 2020

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020 via email

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020 via email

sayanarijit commented Jul 28, 2020

Add option nounset to default to strict parsing. #17

Add option nounset to default to strict parsing. #17

Conversation

sayanarijit commented Jul 28, 2020

codecov-commenter commented Jul 28, 2020 • edited

Codecov Report

mnieber Jul 28, 2020

Choose a reason for hiding this comment

sayanarijit Jul 28, 2020

Choose a reason for hiding this comment

mnieber Jul 28, 2020

Choose a reason for hiding this comment

sayanarijit Jul 28, 2020

Choose a reason for hiding this comment

mnieber commented Jul 28, 2020 via email

sayanarijit commented Jul 28, 2020

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020 via email

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020

sayanarijit commented Jul 28, 2020

mnieber commented Jul 28, 2020 via email

sayanarijit commented Jul 28, 2020

codecov-commenter commented Jul 28, 2020 •

edited