allan2 / dotenvy

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

feat: Implement overriding API methods (prefer #47) #46

Closed LeoniePhiline closed 1 year ago

LeoniePhiline commented 1 year ago

This is a successor to https://github.com/allan2/dotenvy/pull/17

Update: To address and resolve the discussion points below, I am offering another, preferred PR at https://github.com/allan2/dotenvy/pull/47.

This PR


Discussion

Update: To address and resolve these points, I am offering another, preferred PR at https://github.com/allan2/dotenvy/pull/47.

Note that the API is still inconsistent, due to #41:

overload() vs. the more consistent dotenv_overload()

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

Proper naming: Prefer "override" over "overload"

Quoted from https://github.com/allan2/dotenvy/pull/17#issuecomment-1339589211:

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

I disagree. "override" describes the functionality much better than "overload".

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()

Optionally, _with_override() rather than _override().

github-actions[bot] commented 1 year ago

Code Coverage

sonro commented 1 year ago

Good naming is of course difficult.

Indeed, and thank you for taking the time to consider it.

Why mention loading in overload() when the dotenv() function is not named load()?

As per #39 it looks like we are moving towards renaming the function load. In which case load and overload are quite natural from an aesthetic standpoint.

You are right that load_override is more explicit, but overload is consistant with other dotenv libraries. E.g. bkeepers/dotenv.

LeoniePhiline commented 1 year ago

Indeed, and thank you for taking the time to consider it.

Very welcome, and thanks for expressing your appreciation. I strive to help the Rust ecosystem be the best it can be.

As per https://github.com/allan2/dotenvy/discussions/39 it looks like we are moving towards renaming the function load. In which case load and overload are quite natural from an aesthetic standpoint.

There are other points to consider, though:

You are right that load_override is more explicit, but overload is consistant with other dotenv libraries. E.g. bkeepers/dotenv.

Explicit and correct is the Rust wayᵀᴹ.

We should not try to repeat the mistakes of other libraries.

Apparently, Dotenv::Railtie.overload is used primarily to load environment-specific .env files, i.e. using #{Rails.env}. Even though I think their name is unfortunate as well, the comparison is not valid since the function's behavior and use case is different.

Other libraries do not differentiate between a preserving load and an overriding load at all.

Others use parameters but call their load function config() for some reason, despite its global side effects.

Let's try to do things right.


I recommend merging #47, dropping #46, and when #39 comes to fruition, we will rename