search

allan2 / dotenvy

A well-maintained fork of the dotenv crate
MIT License
664 stars 40 forks source link

allow to override existing env vars #41

Closed tshepang closed 1 year ago

tshepang commented 1 year ago

Closes #40

github-actions[bot] commented 1 year ago

Code Coverage

LeoniePhiline commented 1 year ago

@tshepang The test added here does not test overriding, as it does not set an initial env var to override.

It only tests setting env vars.

A negating test for the non-overload behavior is missing as well.

Extensions for the various convenience methods (from filename, from path, from read, ...) are missing.

sonro commented 1 year ago

@LeoniePhiline

The test added here does not test overriding, as it does not set an initial env var to override.

In the overload test, the same var gets set twice. This confirms that the function does indeed override an existing env var.

A negating test for the non-overload behavior is missing as well.

Yes, for completeness this would be nice to have.

Extensions for the various convenience methods (from filename, from path, from read, ...) are missing.

This PR addresses the dotenv functionality only. I do agree it would be useful to extend the overloading behaviour to the other public functions. My main reservation is that this would "bloat" what is otherwise a small library.

LeoniePhiline commented 1 year ago

Tests: Overriding a variable in the same file is a different test case from overriding a variable present in the process environment. Such cases must be tested separately.

Missing methods are not bloat: Library users have the option to read from various sources. If there is an overload functionality, then it makes no sense to restrict that to part of the public API.

Otherwise you would need to argue the from path, from filename and from read methods are bloat as well.

You can always make code opt-in via a feature flag.

My proposal is to revert the merged #41, change #17's force_load into overload (if so preferred by allan2), and then merge #17 instead.

The quality of a general purpose, widely used library is paramount - this places a special burden on maintainers as safeguards of the software's high quality (including, of course, documentation).

sonro commented 1 year ago

Library users have the option to read from various sources. If there is an overload functionality, then it makes no sense to restrict that to part of the public API.

Otherwise you would need to argue the from path, from filename and from read methods are bloat as well.

Good points.