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

Action Controller guide edits for grammar and clarity #18459

Merged
merged 1 commit into from Mar 12, 2015
Merged

Action Controller guide edits for grammar and clarity #18459

merged 1 commit into from Mar 12, 2015

Conversation

cantino
Copy link
Contributor

@cantino cantino commented Jan 12, 2015

This PR updates the Action Controller guide with some minor grammar and clarity improvements.

@arthurnn arthurnn added the docs label Jan 12, 2015
@cantino cantino changed the title Guide edits for grammar and clarity Action Controller guide edits for grammar and clarity Jan 12, 2015

Also, if you've turned on `config.wrap_parameters` in your initializer or calling `wrap_parameters` in your controller, you can safely omit the root element in the JSON parameter. The parameters will be cloned and wrapped in the key according to your controller's name by default. So the above parameter can be written as:
Also, if you've turned on `config.wrap_parameters` in your initializer or called `wrap_parameters` in your controller, you can safely omit the root element in the JSON parameter. In this case, by default, the parameters will be cloned and wrapped with a key chosen based on your controller's name. So the above JSON POST can be written as:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't say "by default," since you're explicitly overriding the default behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, will fix shortly.

@cantino
Copy link
Contributor Author

cantino commented Jan 13, 2015

Thanks @georgeclaghorn, I agree with both of those comments and have updated the PR.

@georgeclaghorn
Copy link
Contributor

👍 You'll also want to squash your commits.

@cantino
Copy link
Contributor Author

cantino commented Jan 13, 2015

✔️ Done!

NOTE: Values such as `[nil]` or `[nil, nil, ...]` in `params` are replaced
with `[]` for security reasons by default. See [Security Guide](security.html#unsafe-query-generation)
for more information.
NOTE: Values such as `[nil]` or `[nil, nil, ...]` in `params` are replaced with `[]` for security reasons by default. See [Security Guide](security.html#unsafe-query-generation) for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this change please ? Even though this seems inconsistent throughout the file, we are slowly wrapping new additions around 80 chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I'll revert this.

@cantino
Copy link
Contributor Author

cantino commented Mar 6, 2015

Thanks @robin850, I've incorporated your suggestions and squashed.


```json
{ "name": "acme", "address": "123 Carrot Street" }
```

And assume that you're sending the data to `CompaniesController`, it would then be wrapped in `:company` key like this:
And, assuming that you're sending the data to `CompaniesController`, it would then be wrapped with a `:company` key like this:
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about "within the :company key" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, "it would then be wrapped within the :company key"? Sure, I can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@cantino
Copy link
Contributor Author

cantino commented Mar 11, 2015

I've rebased and addressed all comments. Is this ready to merge?

@trosborn
Copy link
Contributor

Other than that, everything LGTM!

@@ -131,11 +131,11 @@ To send a hash you include the key name inside the brackets:

When this form is submitted, the value of `params[:client]` will be `{ "name" => "Acme", "phone" => "12345", "address" => { "postcode" => "12345", "city" => "Carrot City" } }`. Note the nested hash in `params[:client][:address]`.

Note that the `params` hash is actually an instance of `ActiveSupport::HashWithIndifferentAccess`, which acts like a hash but lets you use symbols and strings interchangeably as keys.
The `params` hash is actually an instance of `ActiveSupport::HashWithIndifferentAccess`, which acts like a Hash but lets you use symbols and strings interchangeably as keys.
Copy link
Member

Choose a reason for hiding this comment

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

This is not true true anymore. params is an instance of ActionController::Parameters. Maybe we should not talk about which class it inherit but only that symbols and strings are interchangeable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. Changed to

The `params` object acts like a Hash, but lets you use symbols and strings interchangeably as keys.

@cantino
Copy link
Contributor Author

cantino commented Mar 12, 2015

Additional changes made. Thanks @trosborn & @rafaelfranca!

rafaelfranca added a commit that referenced this pull request Mar 12, 2015
Action Controller guide edits for grammar and clarity
@rafaelfranca rafaelfranca merged commit fd3e63e into rails:master Mar 12, 2015
@rafaelfranca
Copy link
Member

Awesome work!

@cantino cantino deleted the action_controller_guide_edits branch March 12, 2015 02:00
@cantino
Copy link
Contributor Author

cantino commented Mar 12, 2015

Thanks!

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

7 participants