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

Add support for --config with JSON file #16

Merged
merged 5 commits into from Oct 8, 2021
Merged

Add support for --config with JSON file #16

merged 5 commits into from Oct 8, 2021

Conversation

menghif
Copy link
Contributor

@menghif menghif commented Oct 5, 2021

Fixes #15

This PR adds a --config option to specify all of the other SSG options in a JSON formatted configuration file.

Example:

node index.js --config options.json

where options.json is equal to:

{
  "input": "./docs",
  "output": "./web"
}

would be the equivalent of doing node index.js -i ./docs -o ./web


I still need to update the README to document the changes.

@lyu4321
Copy link
Owner

lyu4321 commented Oct 6, 2021

Hey @menghif, thanks so much for the changes so far. It looks really good!

Just some small things I wanted to point out:

Styling

  • Using single quotes for consistency
  • "Use a JSON config file" could be "Path to a JSON config file" to match the wording of the other descriptions

Error-handling

  • For the console.error()s that you added, add process.exit(-1) after to exit the program

@menghif
Copy link
Contributor Author

menghif commented Oct 7, 2021

I made the requested changes, except for adding process.exit(-1) because as we discussed it would not allow to reach this line:

console.error('Please see --help for options.');

I also added the following if statement:

if (fs.statSync(argv.config).isFile() && path.extname(input) == '.json') {

to avoid passing a file that is not .json as config file.

@menghif menghif marked this pull request as ready for review October 7, 2021 17:50
index.js Outdated Show resolved Hide resolved
@lyu4321
Copy link
Owner

lyu4321 commented Oct 7, 2021

Hey @menghif sorry for so much back and forth. I tried testing it again and I noticed that it throws an error when the config file doesn't specify an output folder.

For example, if you test using

{
    "input": "docs",
    "stylesheet": "https://cdn.jsdelivr.net/npm/water.css@2/out/water.css",
    "lang": "fr"
}

It throws an error instead of saving in the default 'dist' folder.

One more thing is that if the JSON config file doesn't exist at all, the error message that is printed is 'Input file or folder is required'. I think that a more accurate message would be something like 'JSON config file is required'.

https://github.com/menghif/jellybean/blob/f8eaeb7fa6424bddfa4779d3a471cfbccbe2c558/index.js#L184-L195

You could add an else statement after this code block.

Thank you so much!!!

lyu4321
lyu4321 previously approved these changes Oct 7, 2021
@lyu4321 lyu4321 self-requested a review October 7, 2021 18:39
@lyu4321 lyu4321 dismissed their stale review October 7, 2021 19:01

Accidentally approved

@menghif
Copy link
Contributor Author

menghif commented Oct 8, 2021

I'm really seeing the benefit of code review now! Thank you for your help catching those two problems. I fixed them now! Hopefully it's good now 😊

@lyu4321
Copy link
Owner

lyu4321 commented Oct 8, 2021

@menghif Thanks so much! It looks great, I'll be merging it now :)

@lyu4321 lyu4321 closed this Oct 8, 2021
@lyu4321 lyu4321 reopened this Oct 8, 2021
@lyu4321 lyu4321 merged commit 32eed9a into lyu4321:main Oct 8, 2021
@menghif menghif deleted the issue-15 branch October 8, 2021 13:55
@menghif menghif restored the issue-15 branch October 8, 2021 21:09
@menghif menghif deleted the issue-15 branch October 8, 2021 21:19
@menghif menghif restored the issue-15 branch October 8, 2021 21:26
@menghif menghif deleted the issue-15 branch October 8, 2021 21:27
@menghif menghif restored the issue-15 branch October 8, 2021 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support --config with JSON Config File
2 participants