Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

LeoniePhiline
Copy link

@LeoniePhiline LeoniePhiline commented Dec 6, 2022

This is a successor to #17

Update: To address and resolve the discussion points below, I am offering another, preferred PR at #47.

This PR

  • Adds overriding variants to all relevant public API methods.
  • Fixes documentation.
  • Adds missing test for from_read().
  • Tests non-overriding behavior with greater precision.
  • Tests overriding behavior correctly.
  • Fixes some inconsistent test formatting.

Discussion

Update: To address and resolve these points, I am offering another, preferred PR at #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 #17 (comment):

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

@LeoniePhiline LeoniePhiline changed the title feat: Implement overriding API methods feat: Implement overriding API methods (prefer #47) Dec 6, 2022
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

Code Coverage

@sonro
Copy link
Collaborator

sonro commented Dec 6, 2022

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
Copy link
Author

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 #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:

  • It would still be inconsistent, considering the naming of other parts of this crate's public API.
  • Nothing is being overloaded - this is a misnomer.
  • The problem of sorting order being messed up in the documentation would persist. (See screenshot above.)

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

  • dotenv::dotenv() to dotenv::load(),
  • dotenv::dotenv_iter() to dotenv::load_iter() and
  • dotenv::dotenv_override() to dotenv::load_override().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants