Make config file parsing more strict #46
Conversation
robertbrignull
changed the title
|
@sampart would you be ok taking on review of this PR? However @Daverlo, @anaarmas, and @joshhale you should probably keep an eye on this too though for the reasons I explain below. The main goal here is making the config parsing give more helpful error messages in the case of incorrect input. A side product is throwing an error on any invalid input instead of silently ignoring it in some cases. This is therefore technically not backwards compatible, however only in that it enforces the schema we've already published. I personally think it can still go on the |
sampart
left a comment
sampart
left a comment
There was a problem hiding this comment.
Looks good to me! Just some very minor wording tweaks.
src/config-utils.ts
Outdated
| } | ||
|
|
||
| export function getPathsIgnoreInvalid(configFile: string): string { | ||
| return getConfigFilePropertyError(configFile, PATHS_IGNORE_PROPERTY, 'must be an array of non-empty string'); |
There was a problem hiding this comment.
| return getConfigFilePropertyError(configFile, PATHS_IGNORE_PROPERTY, 'must be an array of non-empty string'); | |
| return getConfigFilePropertyError(configFile, PATHS_IGNORE_PROPERTY, 'must be an array of non-empty strings'); |
src/config-utils.ts
Outdated
| '), a relative path, or of the form owner/repo@ref\n' + | ||
| 'Found: ' + queryUses; | ||
| export function getPathsInvalid(configFile: string): string { | ||
| return getConfigFilePropertyError(configFile, PATHS_PROPERTY, 'must be an array of non-empty string'); |
There was a problem hiding this comment.
| return getConfigFilePropertyError(configFile, PATHS_PROPERTY, 'must be an array of non-empty string'); | |
| return getConfigFilePropertyError(configFile, PATHS_PROPERTY, 'must be an array of non-empty strings'); |
src/config-utils.ts
Outdated
| return getConfigFilePropertyError( | ||
| configFile, | ||
| QUERIES_PROPERTY + '.' + QUERIES_USES_PROPERTY, | ||
| 'is invalid as the local path "' + localPath + '" is output of the repository'); |
There was a problem hiding this comment.
| 'is invalid as the local path "' + localPath + '" is output of the repository'); | |
| 'is invalid as the local path "' + localPath + '" is outside of the repository'); |
|
Thanks for spotting all those 👍 |
This PR aims to make the parsing of the config file stricter, and safer from a code maintenance point of view.
The changes are:
This is obviously going to conflict a bit with #45 but it shouldn't be too bad and it'll be easy to sort out whichever one gets merged first.
Merge / deployment checklist