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

move property name check to property generator #412

Merged
merged 1 commit into from Jul 19, 2019

Conversation

@jannyHou
Copy link
Contributor

commented Jul 18, 2019

Description

connect to https://github.ibm.com/apimesh/apiconnect-cli-loopback/issues/138

APIC cannot invoke the loop of property generator when run the model generator.
In the previous implementation, invoking .composeWith() inside .prompt() cannot finish execution when env.run() takes in a cb function. For details please check the conversation in the related issue.

This PR moves the "property name prompt" and "empty property check" from model generator to property generator, so that the composed generator won't be affected by this.prompt().

Test: we don't test composed generator(actually there is no way to test it), so I manually verified the cli on local:

loopback-cli

installed loopback-cli globally and npm link the generator-loopback inside it

(1) create a model with multiple properties

jannyhous-mbp:lb4app jannyhou$ lb model
? Enter the model name: Foo
? Select the datasource to attach Foo to: db (memory)
? Select model's base class PersistedModel
? Expose Foo via the REST API? Yes
? Custom plural form (used to build REST URL): 
? Common model or server only? common
Let's add some Foo properties now.

Enter an empty property name when done.
? Enter the property name: foo
? Property type: number
? Required? No
? Default value[leave blank for none]: 
property generator emits event finished
model generator receives event finished

Let's add another Foo property.
Enter an empty property name when done.
? Enter the property name: bar
? Property type: string
? Required? No
? Default value[leave blank for none]: 
property generator emits event finished
model generator receives event finished

Let's add another Foo property.
Enter an empty property name when done.
? Enter the property name: 
model generator receives event exitModelGenerator


(2) Add a model without property
jannyhou$ lb model
? Enter the model name: Bar
? Select the datasource to attach Bar to: db (memory)
? Select model's base class PersistedModel
? Expose Bar via the REST API? Yes
? Custom plural form (used to build REST URL): 
? Common model or server only? common
Let's add some Bar properties now.

Enter an empty property name when done.
? Enter the property name: 
model generator receives event exitModelGenerator


(3) add a property 
jannyhous-mbp:lb4app jannyhou$ lb property
? Select the model: Foo
? Enter the property name: baz
? Property type: boolean
? Required? No
? Default value[leave blank for none]: 
property generator emits event finished

(4) the property generator reports error when property name is empty
jannyhous-mbp:lb4app jannyhou$ lb property
? Select the model: Bar
? Enter the property name: 
>> Name is required

apic

(1) add multiple properties when create model
jannyhous-mbp:test jannyhou$ apic create --type model
? Enter the model name: agoodmodel
? Select the data-source to attach agoodmodel to: db (memory)
? Select model's base class PersistedModel
? Expose agoodmodel via the REST API? Yes
? Custom plural form (used to build REST URL): 
? Common model or server only? common
Let's add some agoodmodel properties now.

Enter an empty property name when done.
? Property name: name
   invoke   loopback:property
? Property type: string
? Required? No
? Default value[leave blank for none]: 

Let's add another agoodmodel property.
Enter an empty property name when done.
? Property name: some
   invoke   loopback:property
? Property type: boolean
? Required? No
? Default value[leave blank for none]: 

Let's add another agoodmodel property.
Enter an empty property name when done.
? Property name: 
Done running LoopBack generator

Updating swagger and product definitions
Created /Users/jannyhou/apic-cli-property/test/definitions/test.yaml swagger description
jannyhous-mbp:test jannyhou$ 

(2) add property by `apic loopback:property`
jannyhous-mbp:test jannyhou$ apic loopback:property
? Select the model: agoodmodel
? Enter the property name: foo
? Property type: string
? Required? No
? Default value[leave blank for none]: 
Done running LoopBack generator

Updating swagger and product definitions
Created /Users/jannyhou/apic-cli-property/test/definitions/test.yaml swagger description

(3) `apic loopback:property` reports error when the property name is empty
Created /Users/jannyhou/apic-cli-property/test/definitions/test.yaml swagger description
jannyhous-mbp:test jannyhou$ apic loopback:property
? Select the model: agoodmodel
? Enter the property name: 
>> Name is required

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide
// The model generator test is broken with .composeWith(),
// and we didn't test the composed property generator before anyway.
if (process.env.IS_TEST) return;
const done = this.async;

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 18, 2019

Member

It should be this.async().

@b-admike
Copy link
Member

left a comment

LGTM FWIW even though I'm not familiar with this codebase.

return this.prompt(prompts).then(function(answers) {
debug('answers: %j', answers);
this.name = answers.name || this.name;
// this.name = answers.name || this.name;

This comment has been minimized.

Copy link
@b-admike

b-admike Jul 18, 2019

Member

nitpick: can we remove this commented line?

const emitter = new EventEmitter();

emitter.on('exitModelGenerator', ()=> {
debug('model generator receives event \'exitModelGenerator\'');

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 18, 2019

Member
debug("model generator receives event 'exitModelGenerator'");
// Skip executing the `property` function when run tests.
// The model generator test is broken with .composeWith(),
// and we didn't test the composed property generator before anyway.
if (process.env.IS_TEST) return;

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 18, 2019

Member

Let's use a more specific name such as process.env.LB_CLI_SKIP_PROPERTY.

// as a workaround, an event emitter is introduced to invoke another property prompts
emitter.on('finished', ()=> {
debug('model generator receives event \'finished\'');
this.log(g.f('\nLet\'s add another %s property.', this.displayName));

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 18, 2019

Member

Use " to avoid escaping \'.

@jannyHou jannyHou force-pushed the refactor/property-loop branch from fd619d1 to bb63b3f Jul 19, 2019

@jannyHou jannyHou force-pushed the refactor/property-loop branch from bb63b3f to ee5d4fb Jul 19, 2019

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@raymondfeng @b-admike thanks good catches, fixed them.

@jannyHou jannyHou merged commit 4c9c206 into master Jul 19, 2019

15 checks passed

Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] downstream: apiconnect-cli-loopback@master Success! (ee5d4fb)
Details
[cis-jenkins] downstream: loopback-cli@master Success! (ee5d4fb)
Details
[cis-jenkins] x64 && linux && nvm,10 Success! (ee5d4fb)
Details
[cis-jenkins] x64 && linux && nvm,8 Success! (ee5d4fb)
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
generator-loopback Success! (ee5d4fb)
Details
generator-loopback/node=4.x,os=windows Success! (ee5d4fb)
Details
generator-loopback/node=6.x,os=windows Success! (ee5d4fb)
Details
pr-builder
Details
security/snyk - package.json (StrongLoop) No manifest changes detected

@delete-merged-branch delete-merged-branch bot deleted the refactor/property-loop branch Jul 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.