tonerdo / dotnet-env

A .NET library to load environment variables from .env files
MIT License
427 stars 50 forks source link

refactor LoadOptions #91

Closed Philipp-Binder closed 2 months ago

Philipp-Binder commented 9 months ago

Made some changes to LoadOptions, which allow some usual code-patterns which are currently impossible to use.

  1. allow usage of object-initializer Usually I prefer fluent approaches too, but for options I often prefer to have an object-initializer, which reveals the used options much better than a fluent approach. Especially inside Program.cs I often have a nice combination of fluent vs. object-initializers. Fluent approach is still supported, just added support for object-initializer. Example:

    configurationBuilder
    .AddDotNetEnv(new LoadOptions
    {
        SetEnvVars = false,
        ClobberExistingVars = true,
        OnlyExactPath = false,
    })
    .AddOtherConfigSource();

    The default-values for the propertiers can be moved to a default-ctor if you like this approach better. With simple POCOs I like the default values directly at the property, because this is the first place the IDE sends me to if I check the Property (not the ctor, which is often an additional step for me to determine the default value)

  2. move fluent methods and Load-Methods to an extension Separates the logic from the simple options-POCO. Makes the LoadOptions-class really simple, while still delivering all the functionality (no breaking changes, namespace is the same too, so nothing will break). Separating the Load-Functions is a really good thing I guess, because they coupled the options-object itself to the Env-class. Now the LoadOptions-class has zero dependencies.

  3. add Env.Load()-Override to remove the need for using parameter options by name

  4. add IConfigurationBuilder.AddDotNetEnv()-Override to remove the need for using parameter options by name

There should be no breaking change, so hopefully it just makes life easier by some new allowed code-patterns. And it makes the LoadOptions-Code a bit more readable imho, because it removes the need for the "copy-ctor" for the fluent approach, which additionally reduces the risk of errors if we may add another option. The Options-Object is not "immutable" now, but imho that is more a good thing than a bad one. It makes creating options much easier, for example if you have the settings dependant on some other circumstances (e.g. Environment Prod vs. Develop), which is ugly to implement with this fluent approach.

And I used the @this notation in Extensions again. If you dislike it here too I'm open for another parameter-name, just tell me which one to choose ;)

rogusdev commented 9 months ago

If this is not a breaking change, why are all the examples and tests changed? If people have to change their code to update to this version of the library, it is a breaking change.

If there is new functionality that this change enables, there should be at least one new test demonstrating the new possibilities that were not available before, please.

The code looks different, sure, and maybe that's cleaner, but I don't think this is worth breaking existing code for people if it's just a cleanup.

Philipp-Binder commented 9 months ago

You are absolutely right, that was not intended, I just forgot about the support for the old ctor...

One of the many reasons why Review is a really good thing ;)

I will add the ctor to have it "really" non-breaking.

Philipp-Binder commented 9 months ago

Added Tests for Constructor and for ObjectInitializer. Together they ensure the existence for both now, so no possibility to remove any of them without breaking/changing the tests now ;)

Philipp-Binder commented 9 months ago

And I removed the null-coalescing, with having the ctor again it is better understandable with the defaults in the ctor instead of at the properties itself I think

rogusdev commented 9 months ago

You can and should add tests, and you can change documentation to push people towards a new better approach, since this is not fixing any bugs or anything, but no existing tests should be changed. This change does not seem important enough to make a breaking change for, and in order to prove that this is not a breaking change, all existing tests must pass without modifications. New tests for the new approaches are allowed, but all existing approaches must also still work.

rogusdev commented 2 months ago

After reflection, I do not consider these changes to be improvements, and absolutely not worth breaking backwards compatibility, or even merely risking it.

rogusdev commented 2 months ago

I would reconsider the decline if you can point at a specific thing you want to do, that the current approach genuinely does not allow you to do, or is wildly cumbersome to accomplish, without your changes.

Philipp-Binder commented 2 weeks ago

I completely refactored the approach for refactoring LoadOptions with the usage of the new concept of records.

But since that requires net5.0 I guess that's not the way you want to move. So it's just some information about the new features which are available with latest versions.

Records with a first-class-ctor (ctor which defines Properties directly) are readonly by default (record class, record struct can be marked readonly, but is r/w by default), but it is possible to override whatever is needed.

And with the new with syntax, modifying (means copy+change) is quite easy, which makes the readonly concept extremely flexible without loosing the advantages of readonly.

I gues all the additional functions (NoEnvVars, static and non-static) are no longer needed due to the with-syntax, but I left them for backwards compatibility.