-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(cli): store config in custom file #778
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
Conversation
|
Please note Yeoman already has |
|
I don't see any obvious problems, I am ok to use a custom file name since we want to eventually move away from Yeoman anyways. I'd personally use a file name with an extension, e.g. |
bajtos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please discuss with Raymond what file name to use.
|
@virkt25 There are a few places in the base Yeoman generator to use |
| return this.config.defaults({ | ||
| cliVersion: pkg.version, | ||
| lbVersion: 4, | ||
| lbVersion: 400, // 400 = 4.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep the semver and use semver to check the range compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's impossible to tell if 4110 is 4.11.0 or 4.1.10.
| checkLoopBackProject() { | ||
| if (this.shouldExit()) return false; | ||
| if (this.config.get('lbVersion') >= 400) { | ||
| if (semver.gte(this.config.get('lbVersion'), '4.0.0')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused. Here is what I read: if (storedLbVersion >= '4.0.0') report error and exit. Is it intentional?
BTW, maybe we should use satisfies to check against a range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! For some reason it worked when I ran it in a new scaffolded project.
| '. ' + | ||
| 'The command must be run in a LoopBack project.' | ||
| ); | ||
| if (semver.satisfies(this.config.get('lbVersion'), '< 4.0.0')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs negation
shimks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the tests that I've written for app presence when doing artifact generation are replaced, it should LGTM. There are 6 tests failing at the moment.
555d956 to
34a096b
Compare
|
@slnode test please |
|
@raymondfeng PTAL. |
| semver.satisfies(version, '< 4.0.0') | ||
| ) { | ||
| retErr = new Error( | ||
| 'Invalid version. The command must be run in a LoopBack 4 project.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should report the offending version in the message.
| 'The command must be run in a LoopBack project.' | ||
| let retErr; | ||
|
|
||
| const version = this.config.get('lbVersion'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key is from .yo.rc.json, can we use a more obvious name such as loopback.version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we exit if no lbVersion is found in the config file?
|
@virkt25 what's the status of this pull request? Is it still relevant? Should we close it as abandoned? |
|
Task is relevant but I've not been working on it since it's not in line with the MVP. Closing for now and will re-open when we get around to working on it. |
This creates a file called
.loopbackthat allows us to store project config for future reference. Can store stuff like CLI Version, type of project, orm's being used, etc.Storage API is used for reading / writing to config. Any class extending
BaseGeneratorwill have access to.loopbackconfig.connect to #759