Refactor getting and setting declaration values into utils #5183
Conversation
This change adds two utility functions, `getDeclarationValue` and `setDeclarationValue`, that replace a pattern found across many rule files.
| if (_.has(decl, 'raws.value')) { | ||
| _.set(decl, 'raws.value.raw', value); |
There was a problem hiding this comment.
This change uses lodash because Declaration doesn't strictly adhere to the version of postcss.Declaration that we use. stylelint uses PostCSS 7 and, as best as I can tell, these utilities were written so that support for migration to PostCSS 8 would be easier? Or perhaps the typescript definitions in PostCSS 7 are simply wrong.
The Declaration interface in PostCSS 7 inherits from NodeBase, where raws is defined as NodeRaws and does not support a value property.
In PostCSS 8, there is a declaration.d.ts file that has a DeclarationRaws interface that does supply a type signature that meets the first definition, however.
I thought about creating some complicated @typedefs, but it seemed like we were already using lodash in a few places to get around this type signature problem. Let me know if you have any suggestions on how to define this better, though.
(It's a similar story for using _.get in the getDeclarationValue utility.)
There was a problem hiding this comment.
Great! 👏
I think it would be nice if you could leave the reason as a code comment as below:
// The reason to use Lodash here: ...
// See also: https://github.com/stylelint/stylelint/pull/5183/files#r588047494
if (_.has(decl, 'raws.value')) {
lib/utils/setDeclarationValue.js
Outdated
| * @param {string} value | ||
| * @returns {Declaration} The declaration that was passed in. | ||
| */ | ||
| module.exports = function getDeclarationValue(decl, value) { |
There was a problem hiding this comment.
Oh, this is a typo. 😅
| module.exports = function getDeclarationValue(decl, value) { | |
| module.exports = function setDeclarationValue(decl, value) { |
| if (_.has(decl, 'raws.value')) { | ||
| _.set(decl, 'raws.value.raw', value); |
There was a problem hiding this comment.
Great! 👏
I think it would be nice if you could leave the reason as a code comment as below:
// The reason to use Lodash here: ...
// See also: https://github.com/stylelint/stylelint/pull/5183/files#r588047494
if (_.has(decl, 'raws.value')) {
|
I believe the upload error to Coverall should be resolved by retry: https://github.com/stylelint/stylelint/pull/5183/checks?check_run_id=2037304546#step:7:9 |
jeddy3
changed the title
|
@Dru89 A great refactor, thanks! LGTM, once @ybiquitous suggestions are in. I've restarted the tests. |
|
Thanks, y'all! I've updated the |
This change adds two utility functions,
getDeclarationValueandsetDeclarationValue, that replace a pattern found across many rule files. Extracting thegetValueandsetValuefunctions across the rule files was mentioned by @jeddy3 in #5106. (But if we want a specific issue to track this as well, I can still file one.)I was about to tackle replacing another
style-searchbased rule, but remembered that I'd probably want this utility function. And this change felt large enough to make on its own without also updating an additional rule.