fix: if json variable, use stringified value for reco #551
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes handling of JSON-type variables in the stale variable recommendation system by stringifying JSON values before displaying them in code comments.
Key changes:
- Added JSON stringification for recommended values when variable type is 'JSON'
- Fixed a bug where falsy values (false, 0, "") were incorrectly replaced with defaultValue by changing
||to??operator
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jsalaber
force-pushed
the
COR-3917-use-stringified-json-value-reco
branch
from
414d35d to
559dd35
Compare
jsalaber
force-pushed
the
COR-3917-use-stringified-json-value-reco
branch
from
559dd35 to
d971171
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const indentation = indent ? ' ' : '' | ||
| const jsonString = JSON.stringify(parsed, null, 4) | ||
| return ( | ||
| '\n' + | ||
| jsonString | ||
| .split('\n') | ||
| .map((line) => | ||
| line ? indentation + line : line, | ||
| ) | ||
| .join('\n') + | ||
| '\n' | ||
| ) |
There was a problem hiding this comment.
The indentation logic doesn't add the comment prefix (' * ') that's required for block comments. The blockComment function expects staleWarning to be formatted as a single line with the comment prefix already applied (see line 41 in blockComment). The current implementation adds indentation but not the ' * ' prefix needed for multi-line comment content. This will result in malformed JSDoc comments when the recommended value is a JSON object.
| '\n' | ||
| ) | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
The empty catch block silently ignores JSON parsing errors. While this might be intentional as a fallback mechanism, it would be better to add a comment explaining why parsing errors are being ignored, or at minimum add an empty comment to make it clear the empty block is intentional.
| } catch { | |
| } catch { | |
| // If JSON parsing fails, fall back to using the raw recommendedValue. |
| const value = | ||
| releaseVariation?.variables?.[variable.key] ?? | ||
| variable.defaultValue |
There was a problem hiding this comment.
The change from the logical OR operator (||) to the nullish coalescing operator (??) changes the behavior. The || operator would use defaultValue for any falsy value (0, false, '', null, undefined), while ?? only uses it for null or undefined. This means if releaseVariation?.variables?.[variable.key] is 0, false, or '', the old code would have used defaultValue but the new code will use the falsy value. Ensure this behavioral change is intentional.
Changes