Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Slightly better error for failed tslint.json parses #1686

Merged
merged 3 commits into from
Nov 12, 2016
Merged

Slightly better error for failed tslint.json parses #1686

merged 3 commits into from
Nov 12, 2016

Conversation

JoshuaKGoldberg
Copy link
Contributor

Fixes #1681

configuration's findConfiguration now returns a result wrapper with the attempted path as well as error? and results?. tslint-cli will log the failure nicely.

Fixes #1681

`configuration`'s
`findConfiguration` now returns a result wrapper with
the attempted
`path` as well as `error?` and `results?`. `tslint-cli`
will log the
failure nicely.
Copy link
Contributor

@nchen63 nchen63 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add/amend a test for catching and printing the error? It can go in executableTests.ts

@JoshuaKGoldberg
Copy link
Contributor Author

@nchen63 added

assert.isNotNull(err, "process should exit with error");
assert.strictEqual(err.code, 1, "error code should be 1");

assert.include(stderr, "Invalid option for configuration", "stderr should contain notification about invalid json");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test isn't exercising the code in this PR. Invalid option for configuration is written when the config file is missing. I was expecting Failed to load <path>: <error message>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - that was sloppy. Should be fixed.

@nchen63 nchen63 merged commit 5dff171 into palantir:master Nov 12, 2016
@nchen63
Copy link
Contributor

nchen63 commented Nov 12, 2016

@JoshuaKGoldberg thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants