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

no-duplicate-properties should allow repeat properties when they are directly next to each other #280

Closed
BPScott opened this issue Oct 9, 2015 · 12 comments

Comments

@BPScott
Copy link

BPScott commented Oct 9, 2015

Duplicating properties is sometimes required - such as when providing fallbacks for unsupported features, e.g. the following is a legitimate usage but currently raises a warning:

.foo {
  display: inline-block;
  display: flex;
}

Sass-lint should follow CSSLint's lead and allow duplicate properties when they have different values but are immediately next to each other. I.e. the following are warnings:

/* properties with the same value */
.mybox {
  border: 1px solid black;
  border: 1px solid black;
}

/* properties separated by another property */
.mybox {
  border: 1px solid black;
  color: green;
  border: 1px solid red;
}

But the following should be acceptable, and not raise a warning:

/* one after another with different values */
.mybox {
  border: 1px solid black;
  border: 1px solid red;
}
@DanPurdy
Copy link
Member

DanPurdy commented Oct 9, 2015

I think this should be an extra option, by default it should disallow completely because I think this waters the rule down a lot.

If we had an option such as allow-unique-values or allow-fallback-properties (both awful names I know) I feel this would allow us to dial the rule down rather than start so watered down.

Do we also think that maybe a whitelist of available duplicates as well as them being unique values would be appreciated?

@BPScott
Copy link
Author

BPScott commented Oct 10, 2015

Happy for this to be an option. I agree that strictness by default is preferable. It looks like scss-lint doesn't deal with this either.

We have whitelists in other rules so that sounds handy for consistency. As most of the time needing fallbacks is for display as per my initial example and color things so you can specify a hex color and upgrade to an rgba color, it feels like this is only going to kick in on a subset of properties anyway so limiting that sounds useful.

@DanPurdy
Copy link
Member

Ok I think the whitelist is the way to go then. Checking for unique values may prove too far reaching, Simply adding your properties to an allowed-duplicates whitelist would leave the implementation down to the user but avoid the warnings this rule was meant for.

@Snugug
Copy link
Member

Snugug commented Oct 10, 2015

If we're going to allow a whitelist for this rule, it needs to be a whitelist on the full declaration (display: block, for instance) and not just the property name, as otherwise it provides a large opportunity to a user to shoot themselves in the foot.

@DanPurdy
Copy link
Member

Hmm I don't see that as too practical then really. Font fallbacks for rem's for example is one case where you'd need to whitelist every single pixel size used. This is probably another case where #70 would be useful.

If we were to whitelist just the property name but then still raise a warning/error if the duplicate property wasn't on the next line as above I feel that any major user error here would be avoided?

// even if border is whitelisted then raise an issue here.
.mybox {
  border: 1px solid black;
  color: green;
  border: 1px solid red;
}

Do you think that would be acceptable @Snugug ?

@BPScott
Copy link
Author

BPScott commented Oct 10, 2015

Agreed that #70 might be a preferable solution to whitelisting explicit properties. This sort of defining fallback properties would probably be done within mixins so people wouldn't have to pepper their entire codebase with the ignore comments.

I don't think the whitelist for the full declaration would be feasible, along with @DanPurdy's rem with px fallback, my other most common usage of duplicate properties is for rgba with a hex color fallback which would also require a laundry list of exceptions.

@Snugug
Copy link
Member

Snugug commented Oct 11, 2015

As long as the whitelist is only if they are right next to each other, that's okay by me for a less restrictive whitelist

On Oct 10, 2015, at 7:15 PM, Ben Scott notifications@github.com wrote:

Agreed that #70 might be a preferable solution to whitelisting explicit properties. This sort of defining fallback properties would probably be done within mixins so people wouldn't have to pepper their entire codebase with the ignore comments.

I don't think the whitelist for the full declaration would be feasible, along with @DanPurdy's rem with px fallback, my other most common usage of duplicate properties is for rgba with a hex color fallback which would also require a laundry list of exceptions.


Reply to this email directly or view it on GitHub.

@DanPurdy
Copy link
Member

Agreed

@DanPurdy
Copy link
Member

So... Rather embarrassingly I just went to look at this and there is already an exclusion whitelist option.. It's not as tight an implementation as we just discussed but nonetheless it exists. I will update the documentation with examples and submit a PR to tighten this up.

@mcdado
Copy link

mcdado commented May 20, 2016

Sorry to comment on an old closed issue, but I arrived here after using make-sass-lint-config, receiving the following message:

# The following settings/values are unsupported by sass-lint:
# Linter DuplicateProperty, option "ignore_consecutive"

See scss-lint docs.

I think adding an option that allows properties not because they are in a whitelist but simply because they are right after the other is very convenient and can accomodate many edge-cases (think of calc()).

@DanPurdy
Copy link
Member

Hey @mcdado can you open a separate issue up for this please if you'd like to suggest it as a feature. 👍

@mcdado
Copy link

mcdado commented May 20, 2016

Okay! 😉 #726

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants