Skip to content

Implementation of ReplaceOrCreate - mysql#139

Merged
Amir-61 merged 1 commit into
masterfrom
replace
Mar 28, 2016
Merged

Implementation of ReplaceOrCreate - mysql#139
Amir-61 merged 1 commit into
masterfrom
replace

Conversation

@Amir-61

@Amir-61 Amir-61 commented Dec 8, 2015

Copy link
Copy Markdown
Contributor

Hi @bajtos,

Please note this is just in progress implementation (Very initial implementation); just to make sure I'm on the right track.

Added functionality:

+Implemented replaceOrCreate method for loopback-connector-mysql

Side note:
  • NPM test was failing out of the box.
  • This patch requires changes from other modules including loopback-datasource-juggler, loopback-connector as well.

Connect to a spike task: https://github.com/strongloop-internal/scrum-loopback/issues/621

Thanks

@Amir-61 Amir-61 added the #review label Dec 8, 2015
@Amir-61 Amir-61 assigned Amir-61 and bajtos and unassigned Amir-61 Dec 8, 2015
@Amir-61 Amir-61 changed the title WIP: Initial implementation of ReplaceOrCreate WIP: Initial implementation of ReplaceOrCreate - mysql Dec 9, 2015
@bajtos

bajtos commented Dec 9, 2015

Copy link
Copy Markdown
Member

I don't see any obvious problems, besides the usual duplication of code.

@raymondfeng @superkhau PTAL too

Comment thread lib/mysql.js Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bajtos You proposed to use something like this.modifyOrCreate(model, data, options, this.buildReplaceFields, callback) here; however the buildReplaceFields method has this keyword in it; and it breaks var props = this.getModelDefinition(model).properties inside buildReplaceFields method when we pass buildReplaceFields method as the function argument; how about passing the function name like the way I did here and handeling it in modifyOrCreate?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So there are two ways:

  1. use the function name as you suggested, I think it's less clean because you cannot pass an anonymous function

  2. pass a wrapper function that preserves this. You could use this.buildReplaceFields.bind(this), except that bind has terrible performance. Therefore this would be a more performant solution:

var self = this;
this.modifyOrCreate(model, data, options, function(model, data) { return self.buildReplaceFields(model, data); }, callback);

@Amir-61 Amir-61 assigned Amir-61 and unassigned bajtos Dec 22, 2015
@Amir-61 Amir-61 added this to the #Epic: ReplaceOrCreate milestone Jan 6, 2016
@Amir-61

Amir-61 commented Jan 10, 2016

Copy link
Copy Markdown
Contributor Author

Hey @raymondfeng,

Please also review this patch as well!

I really appreciate your time and feedback :-)

@Amir-61 Amir-61 changed the title WIP: Initial implementation of ReplaceOrCreate - mysql implementation of ReplaceOrCreate - mysql Jan 11, 2016
@Amir-61 Amir-61 changed the title implementation of ReplaceOrCreate - mysql Implementation of ReplaceOrCreate - mysql Jan 11, 2016
@Amir-61 Amir-61 added #wip and removed #review labels Jan 11, 2016
@ghost ghost added #review and removed #wip labels Jan 12, 2016
Comment thread lib/mysql.js Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just clarifying: buildReplaceFields is a protected method implemented in loopback-connector; please see this PR

@Amir-61

Amir-61 commented Jan 14, 2016

Copy link
Copy Markdown
Contributor Author

@superkhau / @raymondfeng,

This is a small patch which is ready for review.

I, also, just re-based and squashed the commits into one.

Thanks for your time!

Comment thread lib/mysql.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

stick to 80 char limit, this one super long line is hard to read.

Comment thread test/mysql.test.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should.equal same for all the ones in the file

@Amir-61
Amir-61 force-pushed the replace branch 2 times, most recently from 273788b to 3eaec8d Compare January 15, 2016 04:01
@superkhau

Copy link
Copy Markdown
Contributor

LGTM

Comment thread lib/mysql.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is an extra space in front of MySQL here

@Amir-61

Amir-61 commented Mar 21, 2016

Copy link
Copy Markdown
Contributor Author

@bajtos I prefer to wait for you to land this patch and this PR since they are related and need to be landed at the same time smoothly to pass CI tests.

Also this PR needs to be landed before landing this patch.

@Amir-61 Amir-61 assigned bajtos and unassigned Amir-61 Mar 21, 2016
@Amir-61
Amir-61 merged commit fe58c75 into master Mar 28, 2016
@Amir-61 Amir-61 removed the #review label Mar 28, 2016
@bajtos
bajtos deleted the replace branch March 29, 2016 13:48
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.

3 participants