Mark inputFilePath as optional in findConfiguration() #3195

lumaxis · 2017-09-02T00:58:36Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

When suppliedConfigFilePath is passed to findConfigurationPath(), inputFilePath is completely ignored and should therefore be optional.

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

[bugfix] Correctly mark inputFilePath as an optional parameter in Configuration.findConfiguration()

palantirtech · 2017-09-02T00:58:37Z

Thanks for your interest in palantir/tslint, @lumaxis! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

ajafff · 2017-09-02T08:18:37Z

src/configuration.ts

@@ -90,7 +90,7 @@ const BUILT_IN_CONFIG = /^tslint:(.*)$/;
 * of the search for a configuration.
 * @returns Load status for a TSLint configuration object
 */
-export function findConfiguration(configFile: string | null, inputFilePath: string): IConfigurationLoadResult {
+export function findConfiguration(configFile: string | null, inputFilePath?: string): IConfigurationLoadResult {


to prevent call like findConfiguration(null, undefined); this function needs overloads. just add these lines above:

export function findConfiguration(configFile: string | null, inputFilePath: string): IConfigurationLoadResult; export function findConfiguration(configFile: string, inputFilePath?: string): IConfigurationLoadResult

Good idea! For some reason I didn't even think of overloads here 🙂

ajafff · 2017-09-02T08:18:48Z

src/configuration.ts

@@ -112,14 +112,14 @@ export function findConfiguration(configFile: string | null, inputFilePath: stri
 * @returns An absolute path to a tslint.json file
 * or undefined if neither can be found.
 */
-export function findConfigurationPath(suppliedConfigFilePath: string | null, inputFilePath: string) {
+export function findConfigurationPath(suppliedConfigFilePath: string | null, inputFilePath?: string) {


same as above

@ajafff I tried adding that here too but then I run into a strange compiler error that I can't really make sense of:

src/configuration.ts(95,58): error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'. Type 'undefined' is not assignable to type 'string'. error Command failed with exit code 2.

From what I understand, this should work but the TS compiler is ignoring the other overload? Not sure. Any ideas? Thanks!

ajafff · 2017-09-02T08:19:52Z

src/configuration.ts

    if (suppliedConfigFilePath != null) {
        if (!fs.existsSync(suppliedConfigFilePath)) {
            throw new FatalError(`Could not find config file at: ${path.resolve(suppliedConfigFilePath)}`);
        } else {
            return path.resolve(suppliedConfigFilePath);
        }
-    } else {
+    } else if (inputFilePath) {


inputFilePath !== undefined to satisfy the linter

Absolutely. Sorry I missed this, thanks!

ajafff · 2017-09-04T06:13:15Z

src/configuration.ts

@@ -90,7 +90,8 @@ const BUILT_IN_CONFIG = /^tslint:(.*)$/;
 * of the search for a configuration.
 * @returns Load status for a TSLint configuration object
 */
-export function findConfiguration(configFile: string | null, inputFilePath: string): IConfigurationLoadResult {
+export function findConfiguration(configFile: string | null, inputFilePath: string): IConfigurationLoadResult;
+export function findConfiguration(configFile: string, inputFilePath?: string): IConfigurationLoadResult {


You actually need 2 overloads and one implementation. Right now there is only 1 overload.

export function findConfiguration(configFile: string | null, inputFilePath: string): IConfigurationLoadResult; export function findConfiguration(configFile: string, inputFilePath?: string): IConfigurationLoadResult; export function findConfiguration(configFile: string | null, inputFilePath?: string): IConfigurationLoadResult { ... }

ajafff · 2017-09-04T06:27:01Z

src/configuration.ts

@@ -90,7 +90,8 @@ const BUILT_IN_CONFIG = /^tslint:(.*)$/;
 * of the search for a configuration.
 * @returns Load status for a TSLint configuration object
 */
-export function findConfiguration(configFile: string | null, inputFilePath: string): IConfigurationLoadResult {
+export function findConfiguration(configFile: string | null, inputFilePath: string): IConfigurationLoadResult;
+export function findConfiguration(configFile: string, inputFilePath?: string): IConfigurationLoadResult {
    const configPath = findConfigurationPath(configFile, inputFilePath);


you will get a compile error on this line, because the parameter types do not match any overload.
You could work around it with this dirty hack:

findConfigurationPath(configFile, inputFilePath!); // note the ! after inputFilePath

Otherwise you need an if..else and still need to use a nonNullAssertion

ajafff · 2017-09-04T06:28:42Z

src/configuration.ts

    if (suppliedConfigFilePath != null) {
        if (!fs.existsSync(suppliedConfigFilePath)) {
            throw new FatalError(`Could not find config file at: ${path.resolve(suppliedConfigFilePath)}`);
        } else {
            return path.resolve(suppliedConfigFilePath);
        }
-    } else {
+    } else if (inputFilePath !== undefined) {


On second thought, you don't need this condition. If suppliedConfigPath === null, then inputFilePath cannot be undefined. You can simply assert that using the nonNullAssertion operator inputFilePath! in the else branch

When `suppliedConfigFilePath` is passed to `findConfigurationPath()`, `inputFilePath` is completely ignored and should therefore be optional

ajafff · 2017-09-12T19:46:47Z

Thanks @lumaxis

Note that github does not send notifications when you force push changes to your branch. I only noticed by chance.

When `suppliedConfigFilePath` is passed to `findConfigurationPath()`, `inputFilePath` is completely ignored and should therefore be optional

lumaxis force-pushed the findConfiguration-optional-parameter branch from aad0683 to ab70d62 Compare September 2, 2017 00:59

ajafff reviewed Sep 2, 2017

View reviewed changes

lumaxis force-pushed the findConfiguration-optional-parameter branch from ab70d62 to 56d646d Compare September 3, 2017 21:34

ajafff reviewed Sep 4, 2017

View reviewed changes

Mark inputFilePath as optional in findConfiguration()

633f47a

When `suppliedConfigFilePath` is passed to `findConfigurationPath()`, `inputFilePath` is completely ignored and should therefore be optional

lumaxis force-pushed the findConfiguration-optional-parameter branch from 56d646d to 633f47a Compare September 4, 2017 21:11

ajafff approved these changes Sep 12, 2017

View reviewed changes

ajafff merged commit 1da80df into palantir:master Sep 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark inputFilePath as optional in findConfiguration() #3195

Mark inputFilePath as optional in findConfiguration() #3195

lumaxis commented Sep 2, 2017

palantirtech commented Sep 2, 2017

ajafff Sep 2, 2017

lumaxis Sep 3, 2017

ajafff Sep 2, 2017

lumaxis Sep 3, 2017

ajafff Sep 2, 2017

lumaxis Sep 3, 2017

ajafff Sep 4, 2017

ajafff Sep 4, 2017

ajafff Sep 4, 2017

ajafff commented Sep 12, 2017

Mark inputFilePath as optional in findConfiguration() #3195

Mark inputFilePath as optional in findConfiguration() #3195

Conversation

lumaxis commented Sep 2, 2017

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech commented Sep 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff commented Sep 12, 2017