Skip to content

Conversation

@jannyHou
Copy link
Contributor

@jannyHou jannyHou commented May 20, 2016

Connect to strongloop/loopback#2292

Contains relation generator fix only.

yeoman-environment uses the latest inquirer, which depends on a new module rx that supports promise only. So we cannot use callback in function checkRelationName

Please review. Thanks.

@jannyHou jannyHou force-pushed the fix/update-to-promise-support branch from 62d9de2 to 6c7d471 Compare May 20, 2016 14:54
* @param {Object} modelDefinition The model which has the relation
* @param {String} name The user input
* @param {Function(String|Boolean)} Callback to receive the check result.
* @returns {String|Boolean} Return the check result.
Copy link
Member

Choose a reason for hiding this comment

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

Please add @promise annotation for strong-docs tooling.

Copy link
Contributor Author

@jannyHou jannyHou May 20, 2016

Choose a reason for hiding this comment

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

@bajtos hmm...i might be wrong, but I think the return value is not a promise, it's just a pure String/Boolean value right?

Copy link
Member

@bajtos bajtos May 23, 2016

Choose a reason for hiding this comment

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

My understanding is that returning a promise (instead of calling a callback) is the whole point of this patch/change, isn't it?

In the code, you do return getAsync().then(..), which returns a promise. You are returning a boolean/string from inside the function passed to then, but that return value is wrapped in a promise for you.

@jannyHou jannyHou force-pushed the fix/update-to-promise-support branch from 9be1557 to c55f803 Compare May 20, 2016 17:59
@jannyHou jannyHou merged commit ba69e64 into master May 20, 2016
@jannyHou jannyHou removed the #review label May 20, 2016
@bajtos bajtos deleted the fix/update-to-promise-support branch May 23, 2016 13:13
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