-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add json-file option to choose a file to be merged with package.json #182
base: master
Are you sure you want to change the base?
Conversation
.parse(process.argv); | ||
|
||
console.log('Demeteorizing application...'); | ||
|
||
Program.output = Program.output || Path.join(process.cwd(), '.demeteorized'); | ||
|
||
if(Program.jsonFile) { |
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.
nitpick: can you amend to add a space after the if
to match style :)
This seems sensible, I'll test locally... |
@desjoerd thanks for the PR! Can you add some tests? |
@HarlanJ there is currently no test coverage for That said, if you want to do the legwork 💯 |
@@ -22,11 +23,19 @@ Program | |||
.option( | |||
'-j, --json <json>', | |||
'JSON data to be merged into the generated package.json') | |||
.option( | |||
'-J, --json-file <jsonFile>' |
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.
missing trailing ,
Sorry about the late comment! I will take a look how I can add some tests. I see two options:
I think option 1 is the best option. What do you guys think? |
@desjoerd sorry for being so late on this. Can you fix the merge conflict so we can look this over again? |
I needed the option to choose a file instead of using "cat" to include the package.json to be merged. This is because I need to use CMD or Powershell which doesn't have "cat" with the same escaping as bash.
I hope that this change will be merged.