Conversation
|
hey @PersonligaPersson sorry for the long due response. YAML support sounds amazing! Happy to merge this PR, would you be able to resolve the conflicts (I have just updated the repo after 2 years)? and I think it would be great to include a small test to try parsing a YAML file, so we can secure this feature and get it tested in the CI. Cheers! |
There was a problem hiding this comment.
Pull request overview
This PR adds YAML support to the .nsprc configuration file, allowing users to write configuration in either YAML or JSON format. The implementation uses the yaml package to parse YAML content and falls back to JSON parsing when appropriate.
Changes:
- Added YAML parsing capability to the file reading utility
- Introduced helper functions to detect and validate both YAML and JSON formats
- Added the
yamlpackage as a dependency
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/utils/file.ts | Updated readFile to support both YAML and JSON formats by detecting file type before parsing |
| src/utils/common.ts | Added isYamlString and getValidStatusAndType functions to validate and detect YAML/JSON formats; updated isJsonString with optional error logging |
| package.json | Added yaml package dependency at version ^2.1.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (logError) { | ||
| console.log('Failed parsing .nsprc file: ' + e); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
The function returns true even when YAML parsing fails (when logError is false). If parsing throws an exception and logError is false, the exception is caught but the function still returns true, incorrectly indicating valid YAML. Add return false; in the catch block before line 31.
| } | |
| } | |
| return false; |
| export function getValidStatusAndType(string: string): Array<boolean> { | ||
| let isYaml = false; | ||
| try { | ||
| if ((isYaml = isYamlString(string, false) || isJsonString(string, false))) { |
There was a problem hiding this comment.
The logic incorrectly determines if content is YAML. The expression isYamlString(string, false) || isJsonString(string, false) assigns the result of the OR operation to isYaml, but since valid JSON is also valid YAML, isYamlString will return true for JSON content, making isYaml always true when either format is valid. This defeats the purpose of distinguishing between formats. Consider checking JSON first, then YAML only if JSON fails.
| * @param {Boolean} logError A boolean that determines if the function should log a caught error to console | ||
| * @return {Boolean} Returns true if the input string is parse-able | ||
| */ | ||
| export function isJsonString(string: string, logError:boolean = true): boolean { |
There was a problem hiding this comment.
Missing space after colon in parameter type annotation. Should be logError: boolean for consistency with TypeScript style conventions.
| * @return {Boolean} Returns true if the input string is parse-able | ||
| */ | ||
| export function isJsonString(string: string): boolean { | ||
| export function isYamlString(string: string, logError:boolean = true): boolean { |
There was a problem hiding this comment.
Missing space after colon in parameter type annotation. Should be logError: boolean for consistency with TypeScript style conventions.
| * @return {Array<Boolean>} The first boolean determines if the input string was valid, the second if it is yaml or not | ||
| */ | ||
| export function getValidStatusAndType(string: string): Array<boolean> { | ||
| let isYaml = false; | ||
| try { | ||
| if ((isYaml = isYamlString(string, false) || isJsonString(string, false))) { | ||
| return [true, isYaml]; | ||
| } | ||
| } catch (e) { | ||
| console.log('Failed parsing .nsprc file: ' + e); | ||
| } | ||
| return [false, false]; |
There was a problem hiding this comment.
The return type Array<boolean> is unclear and error-prone. Consider using a more descriptive return type such as an object with named properties like { isValid: boolean, isYaml: boolean } or a tuple type [isValid: boolean, isYaml: boolean] to make the API more self-documenting.
| * @return {Array<Boolean>} The first boolean determines if the input string was valid, the second if it is yaml or not | |
| */ | |
| export function getValidStatusAndType(string: string): Array<boolean> { | |
| let isYaml = false; | |
| try { | |
| if ((isYaml = isYamlString(string, false) || isJsonString(string, false))) { | |
| return [true, isYaml]; | |
| } | |
| } catch (e) { | |
| console.log('Failed parsing .nsprc file: ' + e); | |
| } | |
| return [false, false]; | |
| * @return {{ isValid: boolean, isYaml: boolean }} An object where `isValid` indicates if the string was parse-able, and `isYaml` indicates if it is YAML or not | |
| */ | |
| export function getValidStatusAndType(string: string): { isValid: boolean; isYaml: boolean } { | |
| let isYaml = false; | |
| try { | |
| if ((isYaml = isYamlString(string, false) || isJsonString(string, false))) { | |
| return { isValid: true, isYaml }; | |
| } | |
| } catch (e) { | |
| console.log('Failed parsing .nsprc file: ' + e); | |
| } | |
| return { isValid: false, isYaml: false }; |
I've added basic support for writing YAML in the nsprc configuration file. This was requested in issue #70.
The basic idea was to use the package yaml to parse the .nsprc file. I've ran this code with the the equivalent nsprc files
1070404: active: true notes: my note expiry: '2022-05-30'and
{ "1070404": { "active": true, "notes": "These are my notes", "expiry": '2022-05-30' } }on a project I'm working on which produced the same output.
Let me know if there are any changes needed for the PR to be accepted. Thanks!