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

[Doc] Add mention to shared section in config_for docs [ci skip] #39099

Merged
merged 1 commit into from May 1, 2020
Merged

[Doc] Add mention to shared section in config_for docs [ci skip] #39099

merged 1 commit into from May 1, 2020

Conversation

albertoalmagro
Copy link
Contributor

Summary

#37913 added the possibility to deeply merge configurations by grouping them within a shared section. This powerful alternative was not reflected in any documentation, which made my team think it was not possible until I found out this feature after looking at the source code.

This patch reflects this change in the documentation so that it is easier for other developers to know about this behavior.

@@ -1617,6 +1617,27 @@ You can also use `Rails::Application.config_for` to load whole configuration fil
Rails.configuration.payment['merchant_id'] # => production_merchant_id or development_merchant_id
```

`Rails::Application.config_for` can also merge configurations deeply. To achieve
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to reword "merge configurations deeply". If I was a beginner I'd have no idea what this means.

I'd reword this to: "Rails::Application.config_for supports a shared configuration to group common configurations. The shared configuration will be merged into the environment configuration." or something similar.

shared:
foo:
bar:
baz: 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
baz: 1
baz: 1

@@ -220,6 +220,22 @@ def message_verifier(verifier_name)
# Rails.application.configure do
# config.middleware.use ExceptionNotifier, config_for(:exception_notification)
# end
#
# # It also merges configurations in a shared section deeply:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same change from above should be made here. We should only say "You can also store configurations in a shared section which will be merged with the environment configuration".

# shared:
# foo:
# bar:
# baz: 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# baz: 1
# baz: 1

37913 added the possibility to deeply merge configurations by grouping
them within a shared section. This powerful alternative was not reflected
in any documentation, which made my team think it was not possible until
I found out this feature after looking at the source code.

This patch reflects this change in the documentation so that it is
easier for other developers to know about this behavior.
Copy link
Contributor Author

@albertoalmagro albertoalmagro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eileencodes for your review! As a non-native English speaker I am more than happy to accept all your suggestions.

@eileencodes eileencodes merged commit 28d815b into rails:master May 1, 2020
@eileencodes
Copy link
Member

Thanks @albertoalmagro!

@albertoalmagro albertoalmagro deleted the add-mention-shared-config-for-documentation branch May 1, 2020 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants