Feature/decouple env vars #105

Philipp-Binder · 2024-09-21T14:39:05Z

Solves #92

Decoupling the concerns of Parsers and Env leads to a more consistent behaviour with different LoadOptions.
Namely it solves the different behaviour of Interpolation, when setting SetEnvVars to true or false.

While the parsers main responsibility now is correct parsing and returning the values from the EnvFile, the Env-class takes the responsibility about setting EnvVars, preventing clobber and so on.

With a next step it would be additionally possible to add a LoadOptions-Parameter to be able to ignore (System-)Environment variables at all, which could be a good thing if you work on a system which is polluted with EnvVars by default, and you don't want to see any side effects from your systems EnvVars.

Philipp-Binder · 2024-09-22T09:51:48Z

Just realized some things which are not covered by tests.
I need to check that first and come back with some new tests showing these cases.

--> converted to draft

Philipp-Binder · 2024-09-28T13:10:08Z

I've reworked the whole draft, putting test-cases to the start of the PR.

But right now I will postpone the work here until the migration to superpower in #106 is completed.

rogusdev · 2024-10-09T16:44:50Z

I like the look of this! Please update for the superpower merge, and I'll go over more granularly. But I very much like where this is headed.

Philipp-Binder · 2024-10-09T18:50:40Z

Updating to superpower-stuff was kinda easy, but still I need to invest some time and comment some cases.
E.g. there are breaking changes to be discussed, I tried to have none, but that was not possible in the end.

I will come back here soon.

Philipp-Binder · 2025-03-02T03:57:28Z

I'm back at this implementation now and rebased it on the current master.
I set it to ReadyForReview again now.

Points to be discussed:

~~new LoadOption IncludeEnvVars~~
- ~~allows to have no "pollution" with system EnvVars~~
~~rename (+obsolete) of NoEnvVars to DoNotSetEnvVars~~
- ~~new name reflects better what the option actually does~~
- ~~should we keep the obsolete name if we introduce breaking changes anyway?~~
breaking change in result of Env.Load() options
- still IEnumerable, but actually it returns a dictionary now
  - should we change it to a Dictionary-Result at all?
  - this is a functional change, because it does not return all pairs as before
    - by returning a dictionary, it can take care of the given clobber-option
- keys from Environment (or additional initial keys) are no longer returned, the result contains only keys from env-files
  - means with non-clobber, that you do not get the non-clobbered key value pair from the initial keys
- ~~if someone needs to retrieve all KeyValuePairs, he can use Parsers.ParseDotEnvFile() directly~~
  - ~~common use case should be dictionary I guess~~
  - ~~using the Parser directly should be the "natural" choice, if you want to have the "raw" results~~
~~breaking change in signature of Parsers.ParseDotEnvFile()~~
- parameters have changed completely, you need to call it with a bool whether to clobber or not, and a dictionary of existing keys value pairs, which should be taken into account (usually Environment Variables)
- ~~this change is necessary to move the logic, all the other method-signatures remain stable though~~

edit: strikethrough parts which were removed from this PR

rogusdev · 2025-03-02T18:10:40Z

Before you go further on this, review existing dot env libraries and their behavior.
https://github.com/bkeepers/dotenv
https://pypi.org/project/python-dotenv/
https://www.npmjs.com/package/dotenv

In particular observe the references to 12 factor apps. And note that the entire point of .env files is to invisibly make the app treat them identically to env vars. Perhaps optional methods to find the specific differences between actual env and .env maybe exist in some implementations (not that I have seen in the past tho), but this is not a library to make people merge env vars with .env files on their own. It does that automatically, and that is the point.

Please consider the intention of these libraries and consider how your vision should fit into that.

Philipp-Binder · 2025-03-02T21:04:44Z

I will have a deeper look, but here my first thoughts without checking the mentioned repos deeper:

The main point seems to be:

the entire point of .env files is to invisibly make the app treat them identically to env vars

I'm not sure what you are aiming at exactly. If it is about the raw values with Parsers.ParseDotEnvFile(), that is something which can be hidden completely to the outside imho. I just wanted to justify the breaking change, since it is public atm.
It is just something someone can use, if he wants to, but the main intention of this Framework should be Env.Load(), or even better ConfigurationSource, which makes the default configuration-magic happen.

Changing the Output of Env.Load() to the resulting dictionary should follow the mindset, since it makes it automatically working, instead of requiring someone to call the correct ToDotEnvDictionary() method afterwards. (which needs to be according to his own settings, which is quite annoying...)

With the last changes I introduced too much. I will revert the new option regarding Environment Variables and maybe bring it with another PR, where we can have a deeper look into that.

Are there any problems with the basics of decoupling only?

Philipp-Binder · 2025-03-02T21:11:17Z

I will update the PR to return non-clobbered env-vars again.
That will remove the breaking change about this aspect.

It will still return only the dictionary, which is the final "Environment"-like result for the user.

And with each change I will update this comment: #105 (comment)

edit: I won't do that change now, see discussion below, that leads more to the point, that the new behaviour should be favored.

Philipp-Binder · 2025-03-07T21:44:20Z

Checking the existing libraries I have two "different ways" in mind:

having 12-factor and simplicity in mind, that's all far too complicated
what we currently have should be "standardized" to dotenvx-definitions, but it should concentrate on "the .Net way"

And there is an additional insight: every repos behaves differently (according to documentation, I did not test it).

Following the first way, many features should just disappear, but that's not the way for this repo I guess.
For a simple, stupid .env-usage, you just need key=value, where value can be quoted for multiline support.
And comments of course, that helps a lot...
But that's it.
All other fancy stuff can be handled by some additional wrapper for example.
With the .Net way of configuration you don't even need multiple files, because that is handled by configuration already, just add multiple configuration-sources, that's what .Net already does with it's own appsettings.json.

That said, coming to the second way, which will be the way of this repo I think.
The best definition I found while checking the existing libraries is dotenvx (Spec: https://dotenvx.com/docs/env-file).
If someone wants to have a language-independent solution (and wants to use npm) I'd say use this one, it seems quite mature, has great documentation, and enables sharing encrypted env-files etc., which is cool if it matches your scenario.

But if a language-specific tool is requested, this repo should serve the best integration possible, which means, support the default .Net way of things.
In my opinion that means favor the use of configuration over directly accessing Environment. Accessing Environment is already solved in the default configuration.
I also found a repo which enables Interploation for Configuration: https://github.com/molinch/ConfigurationSubstitutor
I did not test that so far, but having this separated makes things way more easy. And it works with any configuration, not only with .env files.
I don't actually mean, that we should drop every feature now, I just want to point out, that the .Net way imho should be separation of concerns. And if we can go steps into that direction, we take advantage of the framework, which will help users of this repo in the end.

All of this leads more to a general discussion of where should this repo evolve to. We should move that to a separate discussion instead of this PR I think.

For this PR only, I don't see a point against that change.
You wrote:

this is not a library to make people merge env vars with .env files on their own

I can just guess, that you point at my notes about the breaking change, explicitly these points:

1. keys from Environment (or additional initial keys) are no longer returned, the result contains only keys from env-files

2. if someone needs to retrieve all KeyValuePairs, he can use Parsers.ParseDotEnvFile() directly

The first says, that my new version does NOT return the non-clobbered keys from env, so it actually does not merge env and envFile. The current version does merge them.

The second is about a public method, which can be used if someone wants to, but he does not need to.
As stated in the comments before, from my perspective we can make that private too. But there is no need to, why not allowing someone to inspect the exact parsed key/value-pairs, that does no harm.

I would be happy to know what you see as problematic here.

rogusdev · 2025-03-25T00:58:54Z

src/DotNetEnv/Env.cs

+            var pairs = Parsers.ParseDotenvFile(contents, options.ClobberExistingVars, actualValues);
+
+            // for NoClobber, remove pairs which are exactly contained in previousValues or present in EnvironmentVariables
+            var unClobberedPairs = (options.ClobberExistingVars


Why do you need to check ClobberExistingVars twice? It seems like this should only have to happen once.

Parsers.ParseDotenvFile returns all parsed values, no matter if it clobbers or not.
Interpolation needs clobber-setting to work while parsing, but the responsbility for setting EnvVars should move to the Env-scope, to remove the tight coupling there.
That's why it has to be checked here again.

If we don't want that, we need to return unclobbered values in the parser, but that would change the default Parser-logic, which return all values as IEnumerable --> we need to intercept and return if not clobbered only.

Have a look at the new way with ValueProviders, I think that makes it more precise now.

rogusdev · 2025-03-25T00:59:55Z

src/DotNetEnv/Env.cs

        {
            if (options == null) options = LoadOptions.DEFAULT;

+            previousValues = previousValues?.ToArray() ?? Array.Empty<KeyValuePair<string, string>>();
+
+            var envVarSnapshot = Environment.GetEnvironmentVariables().Cast<DictionaryEntry>()


One of the reasons for previous approaches is to explicitly avoid needing to collect all env vars, and only compare the ones in the file vs the ones out in the wild. It obviously gets weird with interpolation, but the goal is to take a minimal set, rather than get the whole thing, ideally.

Asking this question was kind of a gamechanger, made me think about the concept of ValueProvider.
Snapshotting EnvVars is removed now, and the whole logic is reduced/simplified.

test/DotNetEnv.Tests/Helper/UnicodeChars.cs

rogusdev · 2025-03-25T01:06:34Z

So, I have attempted to absorb this PR a couple times, and overall I think I'm onboard with the direction, but I'd like a breakdown of the changes -- their purpose and implications. Please add comments on major changes throughout the code explaining your thought process and goals. This is a hard one to follow, imo. If we were working a day job together, this is the kind of PR I would want a walkthrough on, so we'll do the best we can here, please :)

rogusdev · 2025-03-25T23:25:31Z

And now you need to rebase with the changes for the new extra interpolation features.

…m SetEnvVars

Shows additional testingIssues due to coupling to EnvironmentVariables (ParseInterpolatedNoEnvVarsTest, ParseInterpolatedTest)

…on when having NoClobber and NoEnvVars

…ess to EnvironmentVariables inside IValue; makes ParseInterploatedNoEnvVarsTest and ParseInterpolatedTest green again breaks many other tests at this stage, because IValue does not check EnvironmentVariables now

…able dependencies in the tests where it is possible (check values differently, to be independent from EnvironmentVariables) makes test AddSourceToBuilderAndLoadMultiWithClobber red, because EnvVarSnapshot gets a reset for each load-process (new multi-file-load problem)

…arsers to EnvironmentVariables)

…epending results with "not to be clobbered" EnvVars in Env.LoadContents makes LoadMultiTestNoEnvVars green

…nvFile by solving the EnvVarSet-Logic completely into Env.LoadContents

…ll tests green;

…ibility for "NoClobber", but if one needs raw values Parsers.ParseDotEnvFile has to be used directly

…aking it just an implementationDetail for LoadMulti

…m Env inside Parsers;

Philipp-Binder · 2025-03-27T21:15:57Z

And now you need to rebase with the changes for the new extra interpolation features.

Done, all tests should be green, but no more time right now to check.

So, I have attempted to absorb this PR a couple times, and overall I think I'm onboard with the direction, but I'd like a breakdown of the changes -- their purpose and implications. Please add comments on major changes throughout the code explaining your thought process and goals. This is a hard one to follow, imo. If we were working a day job together, this is the kind of PR I would want a walkthrough on, so we'll do the best we can here, please :)

Absolutely, I will give some more advice as soon as time is available.

…arates better between Parsed Values and already contained ones --> simplifies Env.LoadContents()

Philipp-Binder · 2025-03-28T21:22:31Z

While making the latest changes I realized, that Parsers is internal.
I made that explicit now, to avoid further confusion.
==> Changes in Parsers are not breaking of course...

I updated the above comment, and there is only this breakng change now:

breaking change in result of Env.Load() options

still IEnumerable, but actually it returns a dictionary now

should we change it to a Dictionary-Result at all?

this is a functional change, because it does not return all pairs as before

by returning a dictionary, it can take care of the given clobber-option

keys from Environment (or additional initial keys) are no longer returned, the result contains only keys from env-files

means with non-clobber, that you do not get the non-clobbered key value pair from the initial keys

Still no time for an adequate walkthrough due to the latest changes ;)
I will provide one next time I find some time.

Philipp-Binder · 2025-03-29T14:54:36Z

Quite much time available at the moment :)
Hope this helps to follow my thoughts:

The Main Goal of the PR is to decouple Parsers/IValue from Environment, and introduce precise responsibilities between Env and Parsers.
The overall goal is to be able to maintain/discuss/extend the single features more independent from each other.

IValueProvider
Helper construct to abstract from Environment and also all other actual value collections.
Chained and KeyValuePair providers are already capable to take clobbering into account, which simplifies keeping a persistent handling of clobber/noClobber.

Parsers
Starting with the FakeEnvVars introduced by you, I moved them to Parsers and changed the concept to usage of the new ValueProvider combined with a Dictionary of the currently parsed values, which allows stable Interpolation with very small effort.
The CurrentValueProvider is recreated for each new call to Parsers.ParseDotenvFile(). It chains the given actualValueProvider and a new ValueProvider for ParsedValues. The complete Provider (itself and the chained ones) take clobbering into account, there is nothing additionally to care about.
Adding values to ParsedValues directly affects the CurrentValueProvider through the IList-reference, so the CurrentValueProvider is always on the current state of parsing.
The local defined method UpdateParsedValues just helps to update the values and return the pair in a single step.
Interpolation works by simply accessing the CurrentValueProvider from IInterpolationHandler now.
No need to know any internals like using Environment or how to take Clobbering into account, since CurrentValueProvider already does that, no "implementation specifics drain" to the outside.

Env
Env only accesses Parsers.ParseDotenvFile() now, instead of accessing delegates for clobber and interpolation handling.
Responsibility for Interpolation is left to Parsers, since that should happen while parsing is in progress, interpolating the values at the "current state" of parsing.
Responsibility for setting Environment is taken bei Env now, Parsers ist completely independent from Environment. That solves #92 by making Interpolation independent from setting Environment.
Env also takes responsibility for Clobbering now. Means that from outside you just get the resulting values which are not clobbering the environment (!breaking change!). That leads to the next point:

EnvConfigurationProvider
Since Env.Load() now takes the Clobbering-Option into account, there is no need to check that additionally.
EnvConfigurationProvider.Load() is reduced to a minimum.

Testing
Testing is simplified, because resetting is as simple as using a new ValueProvider now.
There is no need for exact resets on the actual (Process-)Environment, which actually was no guarantee for "clean" tests, because User and System Environment may contain conflicting variables too.
There are still many tests relying on Environment, but it is not required to do so now. This is something we should reduce even more imho. (except for explicit tests e.g. whether SetEnvVars is working of course)

About the breaking change
If Env.Load() wouldn't take clobbering into account, we'd have to check clobbering in EnvConfigurationProvider again.
It would be possible to introduce another option, which says whether to include or exclude clobbering values if noClobber is checked.
But my opinion is, that you don't really need to get these entries at all, because you wanted them to stay as they were in Environment for a reason I guess.
When using EnvConfigurationProvider the Environment-variables are already contained in the default ConfigurationProviders.
If you handle the whole thing on your own, it is really likely that you have implemented a fallback to Environment already, because you want to access Environment where you don't specify something in your env-file.
Open Question: should we make this breaking change better visible by changing the return-type to a dictionary?

Some words at the end
Imho the separation and responsibilities are still not "perfect", but the PR is a good step in that direction while having just one single breaking change, which hopefully should not lead to actual problems for users if implemented properly (if I miss some kind of usage, just let me know).

Philipp-Binder · 2025-03-29T15:21:43Z

I just read through the PR #110 for more Interpolation-features.
There is a section about net2.0 not supporting empty vs. null behavior, and that's true for Environment of course.
But that's not necessarily true about the new ValueProviders, and it is also not true about ConfigurationProviders.

That is a sample of what we can implement in next steps having this new approach with ValueProviders and separation of concerns.
Feature to be discussed in a different place of course, just saw that and thought this is worth to be mentioned.

…so better understandable without usage of Aggregate here...

Philipp-Binder marked this pull request as draft September 22, 2024 09:50

Philipp-Binder force-pushed the feature/DecoupleEnvVars branch from 385ecc9 to 939a237 Compare September 24, 2024 22:47

Philipp-Binder force-pushed the feature/DecoupleEnvVars branch from 939a237 to f896bea Compare October 9, 2024 18:48

Philipp-Binder force-pushed the feature/DecoupleEnvVars branch from f896bea to dd61524 Compare October 20, 2024 10:10

Philipp-Binder mentioned this pull request Oct 26, 2024

Refactor tests using Theory #108

Merged

Philipp-Binder force-pushed the feature/DecoupleEnvVars branch from dd61524 to 522672a Compare March 2, 2025 01:23

Philipp-Binder marked this pull request as ready for review March 2, 2025 03:57

Philipp-Binder force-pushed the feature/DecoupleEnvVars branch from 0d5928e to b264aba Compare March 2, 2025 21:05

rogusdev reviewed Mar 25, 2025

View reviewed changes

test/DotNetEnv.Tests/Helper/UnicodeChars.cs Show resolved Hide resolved

Philipp-Binder added 8 commits March 27, 2025 21:50

include a green test showing that Interpolation works independent fro…

e576b66

…m SetEnvVars

Introduce red test showing problems with multi-files with NoClobber

98f02d8

Shows additional testingIssues due to coupling to EnvironmentVariables (ParseInterpolatedNoEnvVarsTest, ParseInterpolatedTest)

Introduce red test showing problems with multi-files with Interpolati…

6f51d42

…on when having NoClobber and NoEnvVars

move transformations from Parsers to Env (removes the dependency of P…

daba766

…arsers to EnvironmentVariables)

move EnvVarSnapshot to Parsers, take partial care of clobbering by pr…

a9a5829

…epending results with "not to be clobbered" EnvVars in Env.LoadContents makes LoadMultiTestNoEnvVars green

remove valueAction (former transform Function) from Parsers.ParseDote…

78471ba

…nvFile by solving the EnvVarSet-Logic completely into Env.LoadContents

Philipp-Binder added 5 commits March 27, 2025 22:12

introduce "additionalValues" to solve the multi-file problem; makes a…

0b0c267

…ll tests green;

let Env return only the resulting keyValuePairs --> Env takes respons…

d3bc711

…ibility for "NoClobber", but if one needs raw values Parsers.ParseDotEnvFile has to be used directly

rename additionalValues to previousValues and hide them inside Env, m…

2f5f4c1

…aking it just an implementationDetail for LoadMulti

rename EnvVarSnapshot to ActualValuesSnapshot; move direct usages fro…

9dd5a9b

…m Env inside Parsers;

fix test-declaration and move misplaced comment

1498d20

Philipp-Binder force-pushed the feature/DecoupleEnvVars branch from b264aba to 1498d20 Compare March 27, 2025 21:13

introduce ValueProvider to remove the need for snapshots; it also sep…

9a060b0

…arates better between Parsed Values and already contained ones --> simplifies Env.LoadContents()

refactoring

d53f295

Philipp-Binder force-pushed the feature/DecoupleEnvVars branch from 0fdcbb5 to d53f295 Compare March 29, 2025 14:51

removed unnecessary override for GetValue in ChainedValueProvider; al…

70f8598

…so better understandable without usage of Aggregate here...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/decouple env vars #105

Feature/decouple env vars #105

Philipp-Binder commented Sep 21, 2024 •

edited

Loading

Philipp-Binder commented Sep 22, 2024

Philipp-Binder commented Sep 28, 2024

rogusdev commented Oct 9, 2024

Philipp-Binder commented Oct 9, 2024 •

edited

Loading

Philipp-Binder commented Mar 2, 2025 •

edited

Loading

rogusdev commented Mar 2, 2025

Philipp-Binder commented Mar 2, 2025

Philipp-Binder commented Mar 2, 2025 •

edited

Loading

Philipp-Binder commented Mar 7, 2025

rogusdev Mar 25, 2025

Philipp-Binder Mar 28, 2025

rogusdev Mar 25, 2025 •

edited

Loading

Philipp-Binder Mar 28, 2025

rogusdev commented Mar 25, 2025

rogusdev commented Mar 25, 2025

Philipp-Binder commented Mar 27, 2025

Philipp-Binder commented Mar 28, 2025

Philipp-Binder commented Mar 29, 2025

Philipp-Binder commented Mar 29, 2025

Feature/decouple env vars #105

Are you sure you want to change the base?

Feature/decouple env vars #105

Conversation

Philipp-Binder commented Sep 21, 2024 • edited Loading

Philipp-Binder commented Sep 22, 2024

Philipp-Binder commented Sep 28, 2024

rogusdev commented Oct 9, 2024

Philipp-Binder commented Oct 9, 2024 • edited Loading

Philipp-Binder commented Mar 2, 2025 • edited Loading

rogusdev commented Mar 2, 2025

Philipp-Binder commented Mar 2, 2025

Philipp-Binder commented Mar 2, 2025 • edited Loading

Philipp-Binder commented Mar 7, 2025

rogusdev Mar 25, 2025

Choose a reason for hiding this comment

Philipp-Binder Mar 28, 2025

Choose a reason for hiding this comment

rogusdev Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Philipp-Binder Mar 28, 2025

Choose a reason for hiding this comment

rogusdev commented Mar 25, 2025

rogusdev commented Mar 25, 2025

Philipp-Binder commented Mar 27, 2025

Philipp-Binder commented Mar 28, 2025

Philipp-Binder commented Mar 29, 2025

Philipp-Binder commented Mar 29, 2025

Philipp-Binder commented Sep 21, 2024 •

edited

Loading

Philipp-Binder commented Oct 9, 2024 •

edited

Loading

Philipp-Binder commented Mar 2, 2025 •

edited

Loading

Philipp-Binder commented Mar 2, 2025 •

edited

Loading

rogusdev Mar 25, 2025 •

edited

Loading