Change `shorthand-property-no-redundant-values` report ranges #8008

Mouvedia · 2024-09-25T10:38:47Z

Which issue, if any, is this issue related to?

prepares #7192

Is there anything in the PR that needs further explanation?

see #7192 (comment)

The reported ranges of *-no-vendor-prefix rules will be fixed in a separate PR.

changeset-bot · 2024-09-25T10:38:51Z

🦋 Changeset detected

Latest commit: f476aee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
stylelint	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

romainmenke

Thank you for this effort @Mouvedia,

However I don't think we should be making changes for #7192 at this time.

We should first resolve: #7836

I don't think the changes in this pull request are an improvement for users and I'd rather not make supporting changes for #7192 until we have a good understanding of how to resolve it fully.

If this were an obvious bug fix we could ship it, but I don't think that is the case here.

lib/rules/color-hex-length/index.cjs

lib/rules/shorthand-property-no-redundant-values/index.mjs

Mouvedia · 2024-09-25T12:56:55Z

I don't think the changes in this pull request are an improvement for users […]
If this were an obvious bug fix we could ship it, but I don't think that is the case here.

I disagree for shorthand-property-no-redundant-values, it's an improvement over what we had before.
i.e. the rule name specifically refers to the value hence the value should be reported (not all the declaration)

For color-hex-length it can be discussed.
Either "hex" in the name means hexadecimal or <hex-color>.
i.e. it could be argued that the # is part of "hex"
For me it was obviously hexadecimal length, I guess I was wrong in my assumption.

What about removing the change for color-hex-length and merging the rest?

romainmenke · 2024-09-25T13:25:27Z

For context:

stylelint/src/rules/property-shorthand-no-unnecessary-value/index.js

Lines 96 to 100 in 148a717

    
           report({ 
        
             message: messages.rejected(value, shortestFormString), 
        
             node: decl, 
        
             result, 
        
             ruleName,

This rule has since the beginning always reported both the property name and the value.
That tells us at least that it hasn't been an issue for users and that we don't need to change this :)

But I agree that it is arguably more consistent with other rules.
Is there an existing pattern/guide of when to include the property name and when not to?
Maybe this is just a legacy artifact?

declaration-property-value-no-unknown also only reports the invalid part and not the entire declaration even when it is the value that is invalid within the context of that specific declaration. Same as for shorthand-property-no-redundant-values.

So changing the reported range and message to only the value would bring it in line with declaration-property-value-no-unknown. Maybe there are also counter examples of rules that report the entire declaration?

Making rules more consistent in reported ranges and messages is generally useful and can be done separate from #7192 Such changes would be fine for me :)

For color-hex-length it can be discussed.
Either "hex" in the name means hexadecimal or <hex-color>.
i.e. it could be argued that the # is part of "hex"
For me it was obviously hexadecimal length, I guess I was wrong in my assumption.

Given that the context is CSS colors it can only be <hex-color>, which starts with a # :)

Mouvedia · 2024-09-25T13:33:35Z

Is there an existing pattern/guide of when to include the property name and when not to?

AFAIK we only have a guide for messages.
It's mainly based on common sense and copy/pasting.
#1739 probably had an impact on our previous conventions.
@jeddy3 would know more.

Maybe this is just a legacy artifact?

Probably.

But I agree that it is arguably more consistent with other rules.
Such changes would be fine for me :)

👍

Given that the context is CSS colors it can only be , which starts with a # :)

Alright.
Ill update the table.

…upcoming fix range

romainmenke

@Mouvedia This looks good to me, thank you.
I've left a single suggestions.

lib/rules/shorthand-property-no-redundant-values/__tests__/index.mjs

romainmenke

LGTM

thank you @Mouvedia

@Mouvedia

| datasource | package | from | to | | ---------- | --------- | ------ | ------- | | npm | stylelint | 16.9.0 | 16.10.0 | ## [v16.10.0](https://github.com/stylelint/stylelint/blob/HEAD/CHANGELOG.md#16100) - Changed: `shorthand-property-no-redundant-values` reported ranges ([#8008](stylelint/stylelint#8008)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: `formatter` configuration property ([#7826](stylelint/stylelint#7826)) ([@emmacharp](https://github.com/emmacharp)). - Added: `validate` option to Node.js API and `--validate` CLI flag ([#8009](stylelint/stylelint#8009)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: fixed section to the output of the `verbose` formatter ([#7985](stylelint/stylelint#7985)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string | RegExp` to `ignoreValues` option of `value-no-vendor-prefix` ([#7969](stylelint/stylelint#7969)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string` to `ignoreLonghands` option of `declaration-block-no-redundant-longhand-properties` ([#7971](stylelint/stylelint#7971)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `at-rule-no-unknown` false positives for `@position-try` ([#7968](stylelint/stylelint#7968)) ([@yelizsevinc](https://github.com/yelizsevinc)). - Fixed: `declaration-block-no-redundant-longhand-properties` autofix for `grid-column`/`grid-row` ([#8023](stylelint/stylelint#8023)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `declaration-property-value-no-unknown` false negatives/positives via [`[email protected]`](https://github.com/csstree/csstree/releases/tag/v3.0.0) ([#7993](stylelint/stylelint#7993)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `declaration-property-value-no-unknown` reported ranges and message ([#8003](stylelint/stylelint#8003)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `keyframe-selector-notation` reported ranges ([#7992](stylelint/stylelint#7992)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `length-zero-no-unit` false positives for Sass variables ([#8035](stylelint/stylelint#8035)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `no-descending-specificity` reported ranges ([#7996](stylelint/stylelint#7996)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `selector-anb-no-unmatchable` reported ranges and message ([#8001](stylelint/stylelint#8001)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `string` formatter handling of log symbols for non-Unicode terminals ([#7981](stylelint/stylelint#7981)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: built `.cjs` files to address Rollup vulnerability ([#8010](stylelint/stylelint#8010)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: incorrect `config` option for Node.js API with `extends` and `overrides` ([#8030](stylelint/stylelint#8030)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: stricter validation for the options of `declaration-property-max-values`, `declaration-property-value-no-unknown` and `number-max-precision` ([#7975](stylelint/stylelint#7975)) ([@Mouvedia](https://github.com/Mouvedia)).

@Mouvedia

| datasource | package | from | to | | ---------- | --------- | ------ | ------- | | npm | stylelint | 16.9.0 | 16.10.0 | ## [v16.10.0](https://github.com/stylelint/stylelint/blob/HEAD/CHANGELOG.md#16100) - Changed: `shorthand-property-no-redundant-values` reported ranges ([#8008](stylelint/stylelint#8008)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: `formatter` configuration property ([#7826](stylelint/stylelint#7826)) ([@emmacharp](https://github.com/emmacharp)). - Added: `validate` option to Node.js API and `--validate` CLI flag ([#8009](stylelint/stylelint#8009)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: fixed section to the output of the `verbose` formatter ([#7985](stylelint/stylelint#7985)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string | RegExp` to `ignoreValues` option of `value-no-vendor-prefix` ([#7969](stylelint/stylelint#7969)) ([@Mouvedia](https://github.com/Mouvedia)). - Added: support for `string` to `ignoreLonghands` option of `declaration-block-no-redundant-longhand-properties` ([#7971](stylelint/stylelint#7971)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `at-rule-no-unknown` false positives for `@position-try` ([#7968](stylelint/stylelint#7968)) ([@yelizsevinc](https://github.com/yelizsevinc)). - Fixed: `declaration-block-no-redundant-longhand-properties` autofix for `grid-column`/`grid-row` ([#8023](stylelint/stylelint#8023)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `declaration-property-value-no-unknown` false negatives/positives via [`[email protected]`](https://github.com/csstree/csstree/releases/tag/v3.0.0) ([#7993](stylelint/stylelint#7993)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: `declaration-property-value-no-unknown` reported ranges and message ([#8003](stylelint/stylelint#8003)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `keyframe-selector-notation` reported ranges ([#7992](stylelint/stylelint#7992)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `length-zero-no-unit` false positives for Sass variables ([#8035](stylelint/stylelint#8035)) ([@ryo-manba](https://github.com/ryo-manba)). - Fixed: `no-descending-specificity` reported ranges ([#7996](stylelint/stylelint#7996)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `selector-anb-no-unmatchable` reported ranges and message ([#8001](stylelint/stylelint#8001)) ([@romainmenke](https://github.com/romainmenke)). - Fixed: `string` formatter handling of log symbols for non-Unicode terminals ([#7981](stylelint/stylelint#7981)) ([@Mouvedia](https://github.com/Mouvedia)). - Fixed: built `.cjs` files to address Rollup vulnerability ([#8010](stylelint/stylelint#8010)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: incorrect `config` option for Node.js API with `extends` and `overrides` ([#8030](stylelint/stylelint#8030)) ([@ybiquitous](https://github.com/ybiquitous)). - Fixed: stricter validation for the options of `declaration-property-max-values`, `declaration-property-value-no-unknown` and `number-max-precision` ([#7975](stylelint/stylelint#7975)) ([@Mouvedia](https://github.com/Mouvedia)).

Mouvedia force-pushed the mouvedia-7192-review-1 branch 2 times, most recently from a01ad3d to 6ac31fb Compare September 25, 2024 10:49

Mouvedia changed the title ~~Changed shorthand-property-no-redundant-values and color-hex-length report ranges~~ Change shorthand-property-no-redundant-values and color-hex-length report ranges Sep 25, 2024

Mouvedia force-pushed the mouvedia-7192-review-1 branch from 6ac31fb to 5bac26c Compare September 25, 2024 10:51

Mouvedia requested a review from ybiquitous September 25, 2024 10:54

romainmenke reviewed Sep 25, 2024

View reviewed changes

lib/rules/color-hex-length/index.cjs Outdated Show resolved Hide resolved

lib/rules/shorthand-property-no-redundant-values/index.mjs Show resolved Hide resolved

Align shorthand-property-no-redundant-values report range to their …

c4a329d

…upcoming fix range

Mouvedia force-pushed the mouvedia-7192-review-1 branch from 5bac26c to c4a329d Compare September 25, 2024 13:44

Mouvedia changed the title ~~Change shorthand-property-no-redundant-values and color-hex-length report ranges~~ Change shorthand-property-no-redundant-values report ranges Sep 25, 2024

Mouvedia requested a review from romainmenke September 25, 2024 13:48

romainmenke reviewed Sep 25, 2024

View reviewed changes

lib/rules/shorthand-property-no-redundant-values/__tests__/index.mjs Show resolved Hide resolved

code review 1

f476aee

Mouvedia mentioned this pull request Sep 25, 2024

Add replaced text and corresponding range to Warning type #7192

Open

romainmenke approved these changes Sep 25, 2024

View reviewed changes

Mouvedia merged commit c3ad26a into stylelint:main Sep 26, 2024
17 checks passed

Mouvedia deleted the mouvedia-7192-review-1 branch September 26, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `shorthand-property-no-redundant-values` report ranges #8008

Change `shorthand-property-no-redundant-values` report ranges #8008

Mouvedia commented Sep 25, 2024

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

romainmenke left a comment

Mouvedia commented Sep 25, 2024 •

edited

Loading

romainmenke commented Sep 25, 2024 •

edited

Loading

Mouvedia commented Sep 25, 2024 •

edited

Loading

romainmenke left a comment

romainmenke left a comment

Change shorthand-property-no-redundant-values report ranges #8008

Change shorthand-property-no-redundant-values report ranges #8008

Conversation

Mouvedia commented Sep 25, 2024

changeset-bot bot commented Sep 25, 2024 • edited Loading

🦋 Changeset detected

romainmenke left a comment

Choose a reason for hiding this comment

Mouvedia commented Sep 25, 2024 • edited Loading

romainmenke commented Sep 25, 2024 • edited Loading

Mouvedia commented Sep 25, 2024 • edited Loading

romainmenke left a comment

Choose a reason for hiding this comment

romainmenke left a comment

Choose a reason for hiding this comment

Change `shorthand-property-no-redundant-values` report ranges #8008

Change `shorthand-property-no-redundant-values` report ranges #8008

changeset-bot bot commented Sep 25, 2024 •

edited

Loading

Mouvedia commented Sep 25, 2024 •

edited

Loading

romainmenke commented Sep 25, 2024 •

edited

Loading

Mouvedia commented Sep 25, 2024 •

edited

Loading