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

toJSON fix #211

Merged
merged 5 commits into from
Nov 8, 2017
Merged

toJSON fix #211

merged 5 commits into from
Nov 8, 2017

Conversation

benallfree
Copy link
Contributor

Fix for #210

This feels kind of hacky. The central problem is that Lucid doesn't know how to instantiate a new model. It's done one way in QueryBuilder, another way in create(), and a third way in newUp()

Suggestions?

@coveralls
Copy link

coveralls commented Nov 1, 2017

Coverage Status

Coverage increased (+0.005%) to 96.759% when pulling 125efcf on benallfree:serialize-fix into 05caeb3 on adonisjs:develop.

@@ -408,6 +408,8 @@ class Model extends BaseModel {
static async create (payload, trx) {
const modelInstance = new this()
modelInstance.fill(payload)
modelInstance.$visible = this.visible
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved to _instantiate method.

Copy link
Member

Choose a reason for hiding this comment

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

And will be nice to have a small test around it

@benallfree
Copy link
Contributor Author

Done - please also review the refactoring in _instantiate() and the test I added. The test never failed in the first place so I'm not sure if it's testing what you wanted :)

@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage decreased (-0.02%) to 95.862% when pulling 7a8075e on benallfree:serialize-fix into dfc438c on adonisjs:develop.

this.$frozen = false
this.$visible = null
this.$hidden = null
super._instantiate()
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I am not sure why I even had to instantiate methods at first place. Also I believe it's find to have $visible and $hidden properties on the BaseModel itself and simply remove _instantiate from here.

@thetutlage
Copy link
Member

Rest all looks good to me. Lemme know when to make that final change and we move it to develop

@benallfree
Copy link
Contributor Author

Done. FYI PivotModel uses _instantiate. But yes I'm not sure why it's separate from the constructor :)

Ready to merge.

@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage decreased (-0.02%) to 96.305% when pulling 8757f4a on benallfree:serialize-fix into be987ca on adonisjs:develop.

@thetutlage thetutlage merged commit cbe2cce into adonisjs:develop Nov 8, 2017
@thetutlage
Copy link
Member

Thanks @benallfree.

Also you can please start using https://github.com/commitizen/cz-cli. It is something we use for generating consistent commit messages.

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.

None yet

4 participants