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 InitializeObject hook #247

Merged
merged 1 commit into from
May 15, 2016
Merged

Added InitializeObject hook #247

merged 1 commit into from
May 15, 2016

Conversation

256dpi
Copy link
Contributor

@256dpi 256dpi commented May 15, 2016

Hi there!

First of all thanks for this amazing library!

I'm working on a small very opinionated framework that uses struct tags to simplify the declaration of models. I first thought this might get very complicated with api2go as it uses reflect to create new objects internally. But in fact I learned that most actions go either through crud.Find() or crud.FindAll() which allows to setup the object before jsonapi.Marshal() or jsonapi.Unmarshal() is called. The situation that does not allow such access is on a PATCH request. An object gets created with reflect right before passing it to jsonapi.Unmarshal().

TL;DR:
This pull request adds an optional method that can be implemented to get access to the dynamically created object in a PATCH request before jsonapi.Unmarshal() is called. This is necessary to give context to the models in higher frameworks that abstract the jsonapi interfaces.

Please feel free to change the name of the interface and method. ;)

@coveralls
Copy link

coveralls commented May 15, 2016

Coverage Status

Coverage increased (+5.5%) to 87.153% when pulling c9b73d5 on 256dpi:object-initializer into 0f0aba6 on manyminds:master.

@wwwdata
Copy link
Member

wwwdata commented May 15, 2016

Thanks for the PR. Yes what you are saying makes sense. Im just confused that you mentioned PATCH. You mean POST because you called the potential interface implementation in the Create api route, right? Which makes totally sense because in that, we create a new instance of a struct and then unmarshal into it, which would be the undesired behaviour you are describing.

I currently don't really understand why go 1.4 fails in travis, but I guess it's some golint issues.

I like the interface name, but please squash the fix typo commit into your first, I'll check that the test are working and then we can merge it :)

@wwwdata
Copy link
Member

wwwdata commented May 15, 2016

golang/lint#198

Per the README, golint nowadays requires Go 1.5 or later.

ok, hm, let's ditch that old version 1.4 then :D

@sharpner
Copy link
Member

I would love if you could add a test using the newly introduced behaviour, and additionally shouldn't the interface be something like ObjectInitializer to keep golangs interface guidelines?

@256dpi
Copy link
Contributor Author

256dpi commented May 15, 2016

Thanks for the fast responses.

@wwwdata Ups, yes I mean POST of course.
@sharpner You're right, that makes more sense. I will add a test in a moment.

@wwwdata
Copy link
Member

wwwdata commented May 15, 2016

once #248 is merged you can just rebase on it for the tests to work

@coveralls
Copy link

coveralls commented May 15, 2016

Coverage Status

Coverage increased (+5.7%) to 87.299% when pulling af9c788 on 256dpi:object-initializer into 0f0aba6 on manyminds:master.

@wwwdata
Copy link
Member

wwwdata commented May 15, 2016

👍 just one more rebase before merge

@coveralls
Copy link

coveralls commented May 15, 2016

Coverage Status

Coverage increased (+0.05%) to 87.299% when pulling 619ee96 on 256dpi:object-initializer into d6781fc on manyminds:master.

@wwwdata wwwdata merged commit 5207f05 into manyminds:master May 15, 2016
@256dpi
Copy link
Contributor Author

256dpi commented May 15, 2016

Thanks!

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.

4 participants