Repository navigation
Conversation
|
Wow. Great PR 😊 👍 |
mhemrg
left a comment
mhemrg
left a comment
There was a problem hiding this comment.
I've left a few comments that should be addressed before this gets merged. 😊
| liaraConf = JSON.parse(readFileSync(liaraConfPath)); | ||
| }else{ | ||
| liaraConf = {} | ||
| liaraConf["api_token"] = args.api_token; |
There was a problem hiding this comment.
Using underline in the command line is not a good convention.
I recommend using --api-token.
|
Thank you @HMarzban. Merged. 🎉 |
|
It was my pleasure 😊 |
|
I didn't test this PR myself and seems that it doesn't work properly. |
mhemrg
left a comment
mhemrg
left a comment
There was a problem hiding this comment.
I've left some comments that would help to fix this PR.
| let liaraConf; | ||
| try { | ||
| liaraConf = JSON.parse(readFileSync(liaraConfPath)); | ||
| if(!args.api_token){ |
There was a problem hiding this comment.
Still uses api_token with the underline.
There was a problem hiding this comment.
api_token is default property we get from liaraConf variable when user login successfully.
| "babel-core": "^6.26.0", | ||
| "babel-loader": "^7.1.2", | ||
| "babel-preset-backpack": "^0.4.3", | ||
| "@babel/core": "^7.4.3", |
There was a problem hiding this comment.
Please don't upgrade npm packages unless you're sure that the project is compatible with them.
Currently, the CLI doesn't work with these new versions.
There was a problem hiding this comment.
with the last config of bable-core which was version 6 is not compatible with babel-loader and I face to an error a lot,
now it's compatible with new version of babel-core, and I ran test suite and it was okay.
| liaraConf = JSON.parse(readFileSync(liaraConfPath)); | ||
| }else{ | ||
| liaraConf = {} | ||
| liaraConf["api-token"] = args.api_token; |
There was a problem hiding this comment.
I think this would be correct:
liaraConf.api_token = args["api-token"];
|
I fixed the issue myself :) |
|
Let me correct all these issues and write a new unit test for this PR. then I let you know to check it up all features again. |
2 participants
Hi, as I mentioned in issue #3, I found a way to make it easy deployment for CI/CD.
now we can use
clilike below:If you have any further questions about the concept of implementations, I'll be glad to be accountable 😊