Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix false negatives for custom properties within var() in custom-property-pattern #3655

Closed
StephenEsser opened this issue Sep 11, 2018 · 3 comments · Fixed by #5867
Closed
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule

Comments

@StephenEsser
Copy link

Description

The custom-property-pattern rule is not enforced when applied inline to a property. The lint rule is only enforced when instantiated as a definition.

Many of our variables are defined using an inline fallback pattern so the variables can be themed by downstream applications. Example: background-color: var(custom-property-variable, #fff)

Rule

custom-property-pattern

Example

// Fails lint rule
--invalid-custom-property-variable: #fff;

// Does not fail lint rule
a {
  color: --invalid-custom-property-variable;
}

// Does not fail lint rule
a {
  color: var(--invalid-custom-property-variable, #fff);
}

Reproducible Config

{
  rules: {
    'custom-property-pattern': [
      'foo-.+',
    ],
  },
}

Version

^9.3.0

Expected Behavior

The lint rule should be enforced when custom properties are used in a fallback syntax.

@jeddy3
Copy link
Member

jeddy3 commented Sep 13, 2018

@StephenEsser Thanks for the report

The lint rule is only enforced when instantiated as a definition.

This was the intended behaviour of this rule and the custom-media-pattern one, but I now see how this might not be ideal.

This rule should probably check custom properties that are to be substituted into the value of another property with the var() function. This means the following will be a violation:

a {
  color: var(--invalid-custom-property-variable, #fff);
}

Whereas the following will continue not to be as it's invalid syntax:

a {
  color: --invalid-custom-property-variable;
}

We can, as per our semantic policy, flag this as a bug and do this change in a minor release. Any objections to this approach?


The following is a false positive (as it's invalid syntax) and should be ignored by this rule. We can either squish that bug within this issue or create a separate issue for it.

--invalid-custom-property-variable: #fff;

@jeddy3 jeddy3 changed the title Enforce custom-property-pattern rule for inline definitions False negatives for custom properties within var() in custom-property-pattern Sep 13, 2018
@jeddy3 jeddy3 added the status: needs discussion triage needs further discussion label Sep 13, 2018
@StephenEsser
Copy link
Author

StephenEsser commented Sep 13, 2018

Hi @jeddy3, thanks for getting back on this issue.

I have no objections to that approach, the primary goal of this issue was to enable the lint rule for properties being substituted with the var function.

a {
  color: var(--invalid-custom-property-variable, #fff);
}

The false positive mentioned is not something we're concerned with. It was just one of the ways I found that could get the lint rule to fail for an example.

@jeddy3 jeddy3 changed the title False negatives for custom properties within var() in custom-property-pattern Fix false negatives for custom properties within var() in custom-property-pattern Sep 24, 2018
@jeddy3
Copy link
Member

jeddy3 commented Sep 24, 2018

@StephenEsser As there have been no objections to this proposal. Feel free to contribute a pr to address these false negatives. I suspect you'll want to make sure of postcss-value-parser to parse declaration values and check the pattern against words that are the first child of var functions and begin with --.

@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule good first issue is good for newcomers and removed status: needs discussion triage needs further discussion labels Sep 24, 2018
kawaguchi1102 added a commit to kawaguchi1102/stylelint that referenced this issue Jan 30, 2022
kawaguchi1102 added a commit to kawaguchi1102/stylelint that referenced this issue Feb 6, 2022
kawaguchi1102 added a commit to kawaguchi1102/stylelint that referenced this issue Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: bug a problem with a feature or rule
3 participants