-
Notifications
You must be signed in to change notification settings - Fork 16
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 configuration in package.json
#77
base: master
Are you sure you want to change the base?
Conversation
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.
Thank you very much for the PR! This looks good overall - I do have a few suggestions.
'/.spellcheckerrc.json', | ||
'/.spellcheckerrc.jsonc', |
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.
Ah, good catch!
lib/config/package.ts
Outdated
export function readFromPackage(): ExternalConfig { | ||
let config; | ||
try { | ||
config = JSON.parse(readFileSync('package.json') as unknown as string); |
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 believe readFileSync
returns a string if you pass a character encoding as the second argument:
config = JSON.parse(readFileSync('package.json') as unknown as string); | |
config = JSON.parse(readFileSync('package.json', 'utf-8')); |
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.
Also, we should use the app-root-path
package to construct the file path here. That way, even if users are running spellchecker
from a directory that isn't their project's root directory, we'll still be able to find their package.json
. And we can override the result of appRootPath.resolve('package.json')
in tests, which will make this change easier to test. You can see how we use app-root-path
here:
spellchecker-cli/lib/config/file.ts
Line 42 in 8825f8e
.map(path => appRootPath.resolve(path)) |
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.
Finally I'd suggest using fs.accessSync
to check for the existence of package.json
before calling readFileSync
on it, like we do when loading config files:
spellchecker-cli/lib/config/file.ts
Lines 44 to 48 in 8825f8e
try { | |
accessSync(path); | |
} catch (e) { | |
return false; | |
} |
lib/config/package.ts
Outdated
if ((err as Error).name === 'SyntaxError') { | ||
printError('Unable to parse package.json'); | ||
process.exit(1); | ||
} | ||
return {}; |
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.
Instead of silently falling back to {}
on some errors, I'd rather always print err
, like we're doing here:
spellchecker-cli/lib/config/file.ts
Lines 25 to 26 in 8825f8e
printError(`Failed to read config file at ${filePath}. Error: ${e}`); | |
process.exit(1); |
test/cli-test.ts
Outdated
it ('can read options from `package.json`', async () => { | ||
const result = await runCommand("node build/index.js"); | ||
result.should.not.have.property('code'); | ||
}) |
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.
Nit:
it ('can read options from `package.json`', async () => { | |
const result = await runCommand("node build/index.js"); | |
result.should.not.have.property('code'); | |
}) | |
it ('can read options from `package.json`', async () => { | |
const result = await runCommand("node build/index.js"); | |
result.should.not.have.property('code'); | |
}) |
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 have a suggestion for this tests. I'd like to keep the tests independent from the rest of the project - I don't think they should run on the project's documentation or using the project's package.json
. I'd rather they run using test files in test/fixtures
.
So what I'd suggest is adding a second argument to runWithArguments
, a string?
that can be used to override the value of the APP_ROOT_PATH
environment variable in runCommand
.
Then we'd create a test package.json
in test/fixtures/config
that's independent of the project's package.json
, and change the test to look like:
it ('can read options from `package.json`', async () => { | |
const result = await runCommand("node build/index.js"); | |
result.should.not.have.property('code'); | |
}) | |
it ('can read options from `package.json`', async () => { | |
const result = await runWithArguments('', `${__dirname}/fixtures/config`); | |
result.should.not.have.property('code'); | |
}) |
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 have a suggestion for this tests. I'd like to keep the tests independent from the rest of the project - I don't think they should run on the project's documentation or using the project's package.json. I'd rather they run using test files in test/fixtures.
So what I'd suggest is adding a second argument to runWithArguments, a string? that can be used to override the value of the APP_ROOT_PATH environment variable in runCommand.
Completely agree, working on fix.
I'll be on vacation until Tuesday the 29th - I can take another look at this PR then! Thanks again. |
No problem. Have a good vacation! P.S: still working on fixing the tests, haven't found much time too but definitely will finish soon |
Absolutely, no rush! |
Addresses #73
You can now specify command line arguments from
package.json
.