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

Constructors for Pool and connection types to get configuration from environment #39

Open
abonander opened this issue Jan 9, 2020 · 8 comments
Labels
E-easy enhancement New feature or request

Comments

@abonander
Copy link
Collaborator

TODO: fill this out when I'm not on a phone

@mehcode
Copy link
Member

mehcode commented Jan 12, 2020

I think we were discussing something to this effect (not locked in to the naming):

// Equivalent to: Connection::open(env::var("DATABASE_URL").unwrap_or_default())

Connection::open_from_env()

// Equivalent to: Pool::new(env::var("DATABASE_URL").unwrap_or_default())

Pool::from_env()

// Equivalent to: Pool::builder().build(env::var("DATABASE_URL").unwrap_or_default())

Pool::builder().build_from_env()

@mehcode mehcode added good first issue Good for newcomers enhancement New feature or request labels Jan 12, 2020
@tim-harding
Copy link
Contributor

Started jamming on this a bit tonight, hoping to have some updates early next week or sooner.

@nsanta
Copy link

nsanta commented Mar 9, 2020

Hi. I started to work on this task today and I have two questions:

  • Why Connection::open_from_env() should use open since is deprecated?
  • Why not use the env DATABASE_URL as the default/fallback value instead of create new functions?

@abonander
Copy link
Collaborator Author

That comment was outdated. The new method I believe should be on the Connect trait now and be called connect_with_env().

Why not use the env DATABASE_URL as the default/fallback value instead of create new functions?

Do you mean for Pool::builder().build_from_env()? I feel like going through the builder is already pretty explicit, might as well be explicit where the database URL is coming from.

@abonander
Copy link
Collaborator Author

However, I think we need to discuss this a bit more as we want to combine this with #78, however that implies that the individual connection type defines what parameters it tries to get from the environment; maybe instead the new method on Connect should look something like this:

fn url_from_env() -> crate::Result<Url>;

Where the implementation populates as many parts of the URL as it can (username, password, host, port, database) with information from the environment and then uses published defaults for the rest.

@nsanta
Copy link

nsanta commented Mar 9, 2020

I like the second approach @abonander. Thank you!

@mehcode mehcode added blocked and removed good first issue Good for newcomers labels Apr 8, 2020
@mehcode
Copy link
Member

mehcode commented Apr 8, 2020

Marking this as blocked on doing #174 first

@mehcode
Copy link
Member

mehcode commented May 31, 2020

We now have Connect::connect_with(options) and Connect::connect(url).

I can see adding Connect::connect_from_env().

We should probably also add Pool::new_with(options) and PoolBuilder::build_with(options).

@mehcode mehcode added E-easy and removed blocked labels May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants