Add support for configuration in package.json

[gadhagod]

Addresses #73
You can now specify command line arguments from package.json.

// pacakge.json
{
  "spellchecker": {
    "files": ["README.md"]
  }
}

[tbroadley]

Thank you very much for the PR! This looks good overall - I do have a few suggestions.

[tbroadley]

Ah, good catch!

[tbroadley]

I believe readFileSync returns a string if you pass a character encoding as the second argument:

Suggested change

	config = JSON.parse(readFileSync('package.json') as unknown as string);
	config = JSON.parse(readFileSync('package.json', 'utf-8'));

[tbroadley]

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))

[tbroadley]

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;
	}

[tbroadley]

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);

[tbroadley]

Nit:

Suggested change

	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');
	})

[tbroadley]

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:

Suggested change

	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');
	})

[gadhagod]

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.

[tbroadley]

I'll be on vacation until Tuesday the 29th - I can take another look at this PR then! Thanks again.

[gadhagod]

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

[tbroadley]

Absolutely, no rush!