fix(cli): emit correct property definitions for built-in types in discovered models #4143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jannyHou
reviewed
Nov 18, 2019
packages/cli/test/unit/discover/import-discovered-model.test.js
Outdated
Show resolved
Hide resolved
nabdelgadir
reviewed
Nov 19, 2019
packages/cli/test/unit/discover/import-discovered-model.test.js
Outdated
Show resolved
Hide resolved
|
LGTM so far 👍 |
|
Added one more commit to fix The implementation is not as clean as I would like it to be - we have too many different code paths building model template data - but I guess it's good enough for now. The pull request is ready for final review as far as I am concerned. |
3 tasks
nabdelgadir
approved these changes
Nov 19, 2019
marvinirwin
approved these changes
Nov 20, 2019
Signed-off-by: Miroslav Bajtoš <[email protected]>
… data Extract code converting model definition returned by juggler's discovery function into LB4 model template data, move this code to a new function that can be used independently. Add few basic unit tests to start building fast unit-level test suite verifying how different settings & property definitions are handled by our discovery layer. Signed-off-by: Miroslav Bajtoš <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
Before this change, `lb4 discover` will emit property definitions
similar to this:
@Property({
type: String,
})
title: String;
This commit fixes the behavior to make it consistent with `lb4 model`
and `lb4 import-lb3-models` output:
@Property({
type: 'string',
})
title: string;
Signed-off-by: Miroslav Bajtoš <[email protected]>
Fix `lb4 model` to close the connection to the datasource after the target model was discovered. Before this change, the generator would hang because the connection pool used by some DB connectors was preventing the process from finishing. Signed-off-by: Miroslav Bajtoš <[email protected]>
Before this change, `lb4 model --dataSource` emits property definitions
similar to this:
@Property({
type: String,
})
title: String;
This commit fixes the behavior to make it consistent with `lb4 model`
and `lb4 import-lb3-models` output:
@Property({
type: 'string',
})
title: string;
Signed-off-by: Miroslav Bajtoš <[email protected]>
bajtos
force-pushed
the
fix/discovered-types
branch
from
578bef7
to
e203deb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request fixes model discovery to correctly handle built-in types and convert e.g.
type: Stringtotype: 'string', see #3806.I split the change into several smaller commits:
Add snapshots to CLI tests to give us better understanding of what code we are actually scaffolding. This is also needed to have a test that demonstrates the problem!
Refactor the current implementation to make the fix easier: extract helper function converting discovered model to template data, extract
findBuiltintTypehelper.These first steps are not changing observable behavior. With the preparations done, we can make the fixes now.
Emit correct property definitions for built-in types from
lb4 discover.Disconnect the datasource after the model was discovered by
lb4 model.Emit correct property definitions for built-in types from
lb4 model.Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈