allan2 / dotenvy

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

feat: Add optional API to force load, overriding existing environment variables #17

Closed LeoniePhiline closed 1 year ago

LeoniePhiline commented 2 years ago

I had a use case for .env overriding existing environment variables -- others might have a need for it, too.

I am aware that this feature was not wanted by the maintainer of the original dotenv. Please feel free to close the pull request if you believe the alternate overriding API should not be part of this library.

This is my first contribution proposal to a Rust project - please let me know what I need to adjust, if any.

Note: I had not felt comfortable adding an overriding proc macro to dotenv_codegen_impl yet. It might be useful to add that, though.

sonro commented 1 year ago

Hi @LeoniePhiline thank you for your contribution. Your solution looks sound, but this feature was added to the project separately - I am sorry.

LeoniePhiline commented 1 year ago

@sonro My solution is more complete, and my tests are correct - other than the one merged.

Please consider porting changes from my PR into main reverting #41 and merging #17 instead.

Adjustments for the naming-bikeshed are of course possible if required.

41, which was merged instead, is severely lacking. From the perspective of library users, it is not sufficient.

sonro commented 1 year ago

Hi @LeoniePhiline I am reopening this PR based on your comments made on #41.

Requested changes:

Perhaps you would like to add negating tests for the existing API, although I would understand if you consider this outside the scope of this PR.

allan2 commented 1 year ago

I have read the discussion in #41 and I agree with @sonro's suggestions above. Thanks @LeoniePhiline!

LeoniePhiline commented 1 year ago

Following the naming schema the public fn overload() should be called fn dotenv_overload() -- in line with the crate-wide [base]_[variant]() pub fn-naming scheme, e.g. dotenv() & dotenv_iter().

See also the sorting of public methods in the documentation, where the broken naming scheme in overload() makes it be placed out of context:

image

  • Remove your dotenv_override function.
  • Change "override" to "overload" in your function naming.

Doesn't "override" describe the functionality much better than "overload"?

Good naming is of course difficult. However, I would strongly suggest going forward with "override" rather than "overload". There is no "overloading" happening as is understood in IT. Instead, existing values are overridden by new values. Why mention loading in overload() when the dotenv() function is not named load()?

The following naming makes the most sense:

pub fn dotenv()
pub fn dotenv_iter()
pub fn dotenv_override()

pub fn from_filename()
pub fn from_filename_iter()
pub fn from_filename_override()

pub fn from_path()
pub fn from_path_iter()
pub fn from_path_override()

pub fn from_read()
pub fn from_read_iter()
pub fn from_read_override()

iter::load()
iter::load_override()

Side note: The PR was auto-closed due to branch update. I will re-open in due time.