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.

Sign up for GitHub

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

Jump to bottom

Assert Schema #1677

Merged
merged 3 commits into from
May 17, 2016
Merged

Assert Schema #1677

merged 3 commits into from
May 17, 2016

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Apr 11, 2016

This works, but I'd like some discussionon the interface change, and then I'll
add some tests.

@NullVoxPopuli
Copy link
Contributor

Is this a WIP?

bf4
bf4 reviewed Apr 13, 2016
View reviewed changes
lib/active_model_serializers/test/schema.rb
@@ -68,6 +85,10 @@ def response_body
load_json(response.body)
end

def request_params
request.env['action_dispatch.request.request_parameters']
Copy link
Member Author

Choose a reason for hiding this comment

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

this is defined in Rails. We want it to be only GET params or the POST (raw_params) depending on the request.method, I think. I don't think there's a scenario where they should be merged (as request.params does, IIRC)

@bf4
Copy link
Member Author

bf4 commented Apr 13, 2016

@NullVoxPopuli yes, it's a WIP, though I am using it. the two other bullet points, on reflection , are really JSON API schema concerns, and I don't think they belong here

i.e. JSON API making type optional on post, if that should just be an end-user concern as we develop a schema, and similarly with my other caveat about meta.

@bf4 bf4 force-pushed the allow_non_response_schema_assertions branch from 7e12bc2 to b15a566 Compare April 18, 2016 21:07
@bf4 bf4 force-pushed the allow_non_response_schema_assertions branch from b15a566 to 1cc7e8e Compare May 2, 2016 16:24
@bf4 bf4 force-pushed the allow_non_response_schema_assertions branch from c466dc9 to 3519479 Compare May 17, 2016 15:42
@bf4
Copy link
Member Author

bf4 commented May 17, 2016

updated with changelog

@NullVoxPopuli
Copy link
Contributor

conflicts :-(

@bf4 bf4 force-pushed the allow_non_response_schema_assertions branch from 3519479 to fbd3ab1 Compare May 17, 2016 16:20
@bf4
Copy link
Member Author

bf4 commented May 17, 2016

@NullVoxPopuli updated

@bf4
Copy link
Member Author

bf4 commented May 17, 2016

I've been using this locally for a few weeks now. no complaints

@NullVoxPopuli NullVoxPopuli merged commit 6c321cd into rails-api:master May 17, 2016
maksar
maksar reviewed Aug 7, 2020
View reviewed changes
lib/active_model_serializers/test/schema.rb
@response = response
@payload = payload
Copy link

Choose a reason for hiding this comment

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

line 53 should probably be changed, response_body should be replaced with @payload

Copy link
Member Author

Choose a reason for hiding this comment

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

@maksar PRs welcome.. you just commented on a PR that was closed four years ago

@bf4 bf4 deleted the allow_non_response_schema_assertions branch August 7, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maksar maksar maksar left review comments

Assignees
No one assigned
Labels
C: Feature S: Needs Team Discussion V: 0.10.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bf4 @NullVoxPopuli @maksar