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

Extract common database defaults; better use of YAML #12292

Merged
merged 1 commit into from
Nov 8, 2013

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Sep 19, 2013

Use the power of YAML in the database configurations to better express adapter concerns versus environment concerns. Also see https://github.com/bf4/yaml_resources/blob/master/recipes/rails_database_configuration.md

@bf4
Copy link
Contributor Author

bf4 commented Sep 19, 2013

Let me know if your'e interested in changes in the explanatory text, or if you want more of the common language across the different templates extracted or abstracted.

@bf4
Copy link
Contributor Author

bf4 commented Sep 20, 2013

It would be really super great if when a rails core member reviews this s/he assigns someone to it...

@senny
Copy link
Member

senny commented Sep 27, 2013

/cc @fxn

@bf4
Copy link
Contributor Author

bf4 commented Sep 30, 2013

@senny @fxn Let me know if you'd like me to rebase this off of master

@bf4
Copy link
Contributor Author

bf4 commented Nov 7, 2013

@senny @fxn @tenderlove thoughts?

@fxn
Copy link
Member

fxn commented Nov 7, 2013

On one hand, in my experience it is quite common that different environments share some subset of the configuration. In that sense this edit could be statistically convenient (so to speak).

But on the other hand, it is a smell for me that we need comments to explain to users what's that. The comments are justified because this YAML feature is maybe not so widely known, but that defeats the whole simplification goal in my view, you've only moved the problem. There's redundancy in the current generated YAML, but it is trivial to understand, needs no comments.

So while I sympathize with the proposal, I am not really positive about it.

@bf4
Copy link
Contributor Author

bf4 commented Nov 7, 2013

@fxn thanks for following up.

I wrote this PR because I find myself extracting common configs on every project. This was also featured in Rails Recipe and in a recent post by EnvyLabs also sent in the RubyWeekly. So, there is definitely traction for the idea.

I see the comments as furthering an understanding of a data serialization format that's been popular in Ruby since before _why wrote libsyck in 2003.

I think there is a need to better understand that YAML is more than JSON, and to make use of that for greater developer happiness.

Would you like, perhaps, the PR implemented differently, to be just a comment, but no code change, or perhaps no comments?

@fxn
Copy link
Member

fxn commented Nov 7, 2013

@bf4 from my perspective, the PR would be better without meta comments about YAML, just refactor. It can be argued that if your config is YAML, you have to know YAML.

@bf4
Copy link
Contributor Author

bf4 commented Nov 7, 2013

I'll remove, amend, and force push later tonight. Thanks for your attention

@bf4
Copy link
Contributor Author

bf4 commented Nov 8, 2013

@fxn Updated to remove comments and rebased off of master

@fxn
Copy link
Member

fxn commented Nov 8, 2013

Good to go, thanks for working on this!

fxn added a commit that referenced this pull request Nov 8, 2013
Extract common database defaults; better use of YAML
@fxn fxn merged commit 8ed188c into rails:master Nov 8, 2013
@bf4 bf4 deleted the better_database_yaml branch November 8, 2013 16:11
@bf4
Copy link
Contributor Author

bf4 commented Nov 8, 2013

And thanks for following up @fxn!

@RKushnir
Copy link
Contributor

While dev and test databases often share their settings, production, from my experience, usually has different host and credentials. So besides getting a more cryptic config, there doesn't seem to be any other advantages.

@fxn
Copy link
Member

fxn commented Nov 19, 2013

Your production config will just override a couple of the keys typically.

@bf4
Copy link
Contributor Author

bf4 commented Nov 20, 2013

@fxn Perhaps I should add information on this to the guides.

@RKushnir is there anything in particular you'd find helpful to be documented? Examples?

@fxn
Copy link
Member

fxn commented Nov 20, 2013

@bf4 in principle this change wouldn't need extra documentation in my opinion (other than sync'ing examples to match the file, if there were any).

@bf4
Copy link
Contributor Author

bf4 commented Jan 16, 2017

Followed by #27611

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

4 participants