Enable safe fallback when loading single values

[akaIDIOT]

• needs reviewer(s) to agree with the implementation
• needs changelog entry

Fixes #143

[akaIDIOT]

Not sure this implementation is entirely kosher, thoughts welcome 🤔

This combined with calling it where singular values are expected does actually fairly transparently solve the issue.

[ranieri]

This relies on the formats doing the proper translating of all possible relevant parsing errors back to ValueError, and there not being any extraneous ValueErrors.

I hate to bring this up, but could this be a good place for a custom exception type?

[akaIDIOT]

The only ones not supporting the fallback themselves are JSON and TOML, both using a fairly sane exception hierachy where a parsing failure will raise something that quacks ValueError 😎 Not sure a custom type will add anything there.

[ranieri]

You are right, I was led astray by the complex Exception hierarchy of PyYAML (see for example yaml/pyyaml#750). If the parsers that need it raise ValueErrors, that this should work.

It feels a bit ad-hoc though. If exception types are a part of the Format API, should it be documented so people adding a format can make sure it throws ValueErrors in the cases covered by this fallback?

[akaIDIOT]

Ah yes, documentation of the assumptions here makes sense, let's at least make that clear.

• document the role of ValueError in Format.loadv

[github-actions]

Comparing median (lower is better) metric of benchmarks between this PR's target (562ed1c) and the HEAD of this PR (4d54ee9):

	3.10	3.11	3.12	3.13	3.14	3.15	PyPy 3.10	PyPy 3.11
test_benchmark_get	+2%	-3%	-4%	+0%	-1%	+2%	-2%	+1%
test_benchmark_get_dotted	-1%	-1%	-0%	+2%	-2%	+4%	-3%	+0%
test_benchmark_getattr	+2%	-2%	-3%	+0%	-1%	+3%	+1%	+3%
test_benchmark_getitem	+2%	-2%	-3%	+1%	-2%	+2%	+1%	+2%
test_benchmark_getitem_dotted	-2%	-2%	+0%	+2%	-3%	-0%	-5%	-2%
test_benchmark_init_full_overlap	+2%	+3%	-1%	-1%	-2%	+0%	-2%	+1%
test_benchmark_init_no_overlap	+2%	+1%	-0%	-2%	-2%	-1%	+0%	+2%
test_benchmark_init_partial_overlap	+3%	+3%	-1%	-0%	-1%	+1%	-2%	+2%
test_benchmark_match_mapping	+2%	-1%	-4%	-0%	-1%	+2%	+1%	-0%
test_benchmark_reference_chain	-2%	+1%	-2%	-3%	+2%	+1%	-1%	+2%
test_benchmark_splat_args	+1%	+6%	+0%	-0%	+1%	+0%	+5%	+8%
test_benchmark_spread_kwargs	-2%	+2%	+1%	-1%	-1%	+4%	+2%	-0%

(This comment will be updated on subsequent pushes)

[akaIDIOT]

Changing this needs a test

[akaIDIOT]

Alternatively, we could so something like Format(strict=False), where the fallback type is always str. Less explicit, maybe easier to understand?

[ranieri]

Realistically, what do we expect this callable to be/do? Are there many more sane options than str? Do we expect the behavior to change per format?

I'm thinking this might be too much flexibility for our needs.

[akaIDIOT]

Agreed, str is the one and only sane option I can think of at the moment, really. So that actually doesn't need to be parametrized. The behaviour wouldn't need to change per format, for so far as the formats behave the same way (as in: YAML doesn't really need these hoops as it's (too?) flexible in itself).

In hindsight, strict might not be a great fit either though, which begs the question: should this feature (if we go through with it) even get a switch or be enabled by default?

[ranieri]

Based on the current functionality and the current implementation, I think we have two (not trivially reconcilable) requirements:

1. APP_PARENT_KEY=bare-string-value should regardless of the format
2. APP_PARENT_KEY2=<... document of FORMAT...> should work for the chosen format.

The issues arise from the fact that 1 is different from 2 for non-yaml formats.

Possible solutions include:

1. Force users to specify strings in a format-compatible way, ie. appropriate quoting. (define away the issue)
2. Parse env with a function that falls back to the bare string value on failure (this PR, sort of)
3. Fall back on other format(s) (YAML) when parsing names, files or values (env specific or no.)
4. Somehow detect/pass the format per individual file/var.
5. Strike one of the two requirements (probably 2)

Solution 3 can be very surprising, and not in a good way, if there's an actual syntax error in the document! Why am i getting an error in format X instead of Y?

Solution 4 is complex and error prone. Do not recommend.

Realistically we're left with 1, 2 and 5.

Solution 1 clearly didn't work well since we're in this PR considering this change, and 5 would dump one of the main reasons to use confidence over bare env vars.

So we're stuck with some variation of 2, it seems!

[akaIDIOT]

Possible solutions include:

1. Force users to specify strings in a format-compatible way, ie. appropriate quoting. (define away the issue)

Yeah, that's the current state of things without this PR (see quoting stuff in the test code for citatio.

2. Parse env with a function that falls back to the bare string value on failure (this PR, sort of)

Correct, details still under consideration, but yes.

3. Fall back on other format(s) (YAML) when parsing names, files or values (env specific or no.)

100% agreed that this is a terrible idea, particularly with requirement 2 in mind; having to switch syntax is surprising and cumbersome.

4. Somehow detect/pass the format per individual file/var.

Feels like the wrong kind of magic and indeed, also complex and error prone. Let's not :)

5. Strike one of the two requirements (probably 2)

That's actually not really the hard one, I think. The common use case is putting a string in an env var, and even that already breaks in the current state of things. Fixing that while still enabling other simple values like numbers or booleans would still require something along the lines of this PR. The second requirement is almost a side effect of supporting non-string simple values.

Realistically we're left with 1, 2 and 5.

Solution 1 clearly didn't work well since we're in this PR considering this change, and 5 would dump one of the main reasons to use confidence over bare env vars.

So we're stuck with some variation of 2, it seems!

That's exactly my train of thought too, so thanks for arriving at that same point 👯‍♂️

Aside from agreeing, I'm still on the fence whether this should be enabled by default, as the most surprising things this will be doing is silently eating syntax errors in environment variables. Should someone put a full document into an env var and make a syntax mistake in it, it will become a long string without any loud errors at load time. Another way this could be parametrized is read_envvars in the load order; the default implementation would only accept simple values, while a variant with a specific flag, or a customized load order would accept full documents? Again, that's not a full solution, but it might require some consideration on the user side of this 🤔