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

Added testcases for serializable_resource and fixed a documentation error regarding adapter key constant #1501

Merged
merged 1 commit into from
Feb 8, 2016

Conversation

domitian
Copy link
Contributor

@domitian domitian commented Feb 7, 2016

Fixed a documentation error regarding adapter key constant, added a new test cases for use_adapter? method in serializable resource object and fixed a documentation typo.

@domitian domitian changed the title Fixed a documentation error regarding adapter key constant, added testcases Added testcases for serializable_resource and fixed a documentation error regarding adapter key constant Feb 7, 2016
@@ -64,7 +64,7 @@ Details:
1. **ActionController::Serialization**
1. `serializable_resource = ActiveModel::SerializableResource.new(resource, options)`
1. `options` are partitioned into `adapter_opts` and everything else (`serializer_opts`).
The adapter options keys for the are defined by `ADAPTER_OPTIONS`.
The adapter_opt keys are defined in `ActiveModel::SerializableResource::ADAPTER_OPTION_KEYS`.
Copy link
Member

Choose a reason for hiding this comment

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

adapter_opts

@domitian
Copy link
Contributor Author

domitian commented Feb 8, 2016

@bf4 done, removed the additional testcase.

@bf4
Copy link
Member

bf4 commented Feb 8, 2016

Alright, squash commits, add yourself to the top of the fixes changelog and looks good to merge

@domitian domitian force-pushed the testcases-documentation-fix branch from 42d329b to f1de1bf Compare February 8, 2016 06:38
@domitian
Copy link
Contributor Author

domitian commented Feb 8, 2016

@bf4 done.

@bf4
Copy link
Member

bf4 commented Feb 8, 2016

@domitian Thanks. Commit message is a little long, but is ok. For a laugh at an egregious offender, see http://stopwritingramblingcommitmessages.com/ :)

@@ -23,5 +23,13 @@ def test_serializable_resource_delegates_as_json_to_the_adapter
options = nil
assert_equal @adapter.as_json(options), @serializable_resource.as_json(options)
end

def test_use_adapter_with_adapter_option
assert_equal ActiveModel::SerializableResource.new(@resource, { adapter: 'json' }).use_adapter?, true
Copy link
Member

Choose a reason for hiding this comment

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

oh, missed the expectation here. it's assert_equal(expected, actual). But in this case, since it's boolean, it should just be assert and in the next test refute

@domitian domitian force-pushed the testcases-documentation-fix branch from f1de1bf to b8016da Compare February 8, 2016 07:22
@domitian
Copy link
Contributor Author

domitian commented Feb 8, 2016

@bf4 Made the changes suggested(reduced the commit message length, using refute and assert in boolean cases.
And all in all, thanks for your patience in helping me with this PR. Learnt a lot.

@domitian domitian force-pushed the testcases-documentation-fix branch from b8016da to 68f09e5 Compare February 8, 2016 09:58
@domitian
Copy link
Contributor Author

domitian commented Feb 8, 2016

I think this is done and ready to be merged.

bf4 added a commit that referenced this pull request Feb 8, 2016
Added testcases for serializable_resource and fixed a documentation error regarding adapter key constant
@bf4 bf4 merged commit 020db79 into rails-api:master Feb 8, 2016
@bf4
Copy link
Member

bf4 commented Feb 8, 2016

Yay!

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.

2 participants