Uh oh!
There was an error while loading. Please reload this page.
[SEMVER-MAJOR] Discard sugar method for model creation - #2401
Conversation
gunjpan
commented
Jun 3, 2016
@bajtos : Please review, thank you. |
gunjpan
commented
Jun 6, 2016
@slnode test please |
| `app.model(modelName, settings)`, a sugar for creating non-existing model, is | ||
| now removed in favor of promoting use of: | ||
| - `app.registry.createModel(modelName, properties, options) to create new model |
@gunjpan reviewed, PTAL. |
| if (typeof Model === 'string') { | ||
| var msg = 'app.model(modelName, settings) is no longer supported. ' + | ||
| 'Use app.registry.createModel(modelName, definition) and ' + | ||
| 'app.model(modelCtor, config) instead.'; |
There was a problem hiding this comment.
ModelCtor please, we usually use Pascal Case for constructors (class names).
bajtos
commented
Jun 8, 2016
@gunjpan two more comments, otherwise the patch LGTM.
No further review is necessary, please squash the commits before landing. |
gunjpan
commented
Jun 10, 2016
@slnode test please |
08b273c to
cffb4f1Comparegunjpan
commented
Jul 12, 2016
@slnode test please |
1 similar comment
gunjpan
commented
Aug 4, 2016
@slnode test please |
80cd52f to
bc0b6f7Compare| app.set('remoting', { errorHandler: { debug: true, log: false }}); | ||
| app.use(loopback.rest()); | ||
| app.enableAuth(); | ||
| app.dataSource('test', { connector: 'memory' }); |
There was a problem hiding this comment.
I think we misunderstood each other. Keep the app creation in the before hook, because that part is typically shared by all test. Keep the line var MyTestModel = ... in the test, because the model is specific to the test case.
There was a problem hiding this comment.
@bajtos : In the previous review cycle, IIRC, (can't quote commit as rebased), I had kept the before() part, but was advised to remove it as there is only one case in that describe() block.
Do you still recommend changing it?
There was a problem hiding this comment.
I see. Let's not bike shed and keep the code as it is now.
bajtos
commented
Aug 16, 2016
@gunjpan LGTM |
bajtos
commented
Aug 16, 2016
@gunjpan please rebase on top of the current master and check CI results before landing. |
gunjpan
commented
Aug 16, 2016
@slnode test please |
bc0b6f7 to
214269eCompare214269e to
2f3b78aCompareCurrent implementation of `app.model(modelName, settings)` works as a sugar for model creation. In 3.0, this is not supported anymore. This implementation reports an error when sugar is used for model creation. Includes: - Updated app.model() method - Fixed test cases reflecting the change
2f3b78a to
832e2c3Comparebajtos
commented
Sep 7, 2016
@gunjpan I have rebased your patch on top of the latest master. All CI builds are passing with the exception of I am going to land this patch. |
Current implementation of `app.model(modelName, settings)` works as a sugar for model creation. In LB 3.0, this is not supported anymore. This backporting: - keeps the sugar method for model creation for backward compatibility - updates test cases to use `app.registry.createModel()` for model creation Backport of #2401
Current implementation of `app.model(modelName, settings)` works as a sugar for model creation. In LB 3.0, this is not supported anymore. This backporting: - keeps the sugar method for model creation for backward compatibility - updates test cases to use `app.registry.createModel()` for model creation Backport of #2401
Current implementation of `app.model(modelName, settings)` works as a sugar for model creation. In LB 3.0, this is not supported anymore. This backporting: - keeps the sugar method for model creation for backward compatibility - updates test cases to use `app.registry.createModel()` for model creation Backport of #2401
Current implementation of
app.model(modelName, settings)works as a sugar for model creation. In 3.0, this is
not supported anymore. This implementation reports an
error when sugar is used for model creation.
Includes:
Connect to https://github.com/strongloop-internal/scrum-loopback/issues/772