Allow empty entries in property-sort-order declaration indicating visual separation of property blocks

[gajus]

From scss-lint PropertySortOrder:

You can enforce that "groups" of properties be visually separated by setting the separate_groups option to true. When specifying a custom order, you can indicate that you want two groups of properties to be visually separate by inserting an empty item, e.g.

linters:
  PropertySortOrder:
    order:
      - display
      - position
      -            # This empty element signals a visual separation
      - margin
      - padding
    separate_groups: true

which requires that CSS properties are grouped, e.g.

display: block;

maring: 0;

It would be great if property-sort-order supported this. If it does already, it is not documented anywhere.

[DanPurdy]

Thanks for this.

There was discussion about this already i believe in #350

[gajus]

@DanPurdy I am writing a PR for this.

There is one condition that I cannot understand:

sass-lint/lib/rules/property-sort-order.js

Lines 35 to 72 in 728c07f

	if (typeof order === 'string') {
	if (order === 'alphabetical') {
	keys = keys.sort();
	}
	}
	else if (typeof order === 'object') {
	var orderedKeys = [];
	for (i = 0; i < order.length; i++) {
	if (keys.indexOf(order[i]) !== -1) {
	orderedKeys.push(order[i]);
	}
	}
	keys = orderedKeys;
	}
	else {
	keys = keys.sort(function (a, b) {
	if (order.indexOf(a) === -1) {
	if (unknown.indexOf(a) === -1) {
	unknown.push(a);
	}
	}
	if (order.indexOf(b) === -1) {
	if (unknown.indexOf(b) === -1) {
	unknown.push(b);
	}
	}
	if (order.indexOf(a) > order.indexOf(b)) {
	return 1;
	}
	if (order.indexOf(a) < order.indexOf(b)) {
	return -1;
	}
	return 0;
	});
	}

First you check if order is a string. Then you check if order is an object.

The else { block will never be reached? (what else can order be?)

[gajus]

Writing expectations in such a way makes 0 sense.

https://github.com/sasstools/sass-lint/blob/develop/tests/rules/property-sort-order.js#L249

When a single test breaks, all the other tests break too. And you have zero clue as to what is the expectation of the error.

[gajus]

Either my or your implementation of property-sort-order is wrong, because all of the tests are failing.

I have isolated an example.

Here is the subject of the test:

.other-property {
  composes: heading;
  height: 100vh;
  display: block;
  width: 100vw;
  border: 1px;
}

Here is the test:

it.only('[order: custom]', function (done) {
  lint.test(file, {
    'property-sort-order': [
      1,
      {
        'order': [
          'height',
          'width',
          'display',
          'color'
        ]
      }
    ]
  }, function (data) {
    console.log( JSON.stringify(data, '', 4) );

    lint.assert.equal(8, data.warningCount);
    done();
  });
});

Here is the error that my script produces:

{
    "filePath": "property-sort-order.scss",
    "warningCount": 2,
    "errorCount": 0,
    "messages": [
        {
            "ruleId": "property-sort-order",
            "line": 4,
            "column": 3,
            "message": "Expected `width`, found `display`",
            "severity": 1
        },
        {
            "ruleId": "property-sort-order",
            "line": 5,
            "column": 3,
            "message": "Expected `display`, found `width`",
            "severity": 1
        }
    ]
}

Here is an error that your script produces:

{
    "filePath": "property-sort-order.scss",
    "warningCount": 2,
    "errorCount": 0,
    "messages": [
        {
            "ruleId": "property-sort-order",
            "line": 2,
            "column": 3,
            "message": "Expected `height`, found `composes`",
            "severity": 1
        },
        {
            "ruleId": "property-sort-order",
            "line": 3,
            "column": 3,
            "message": "Expected `width`, found `height`",
            "severity": 1
        }
    ]
}

[gajus]

I did find an error in my code.

That brings my error report closer to yours:

{
    "filePath": "property-sort-order.scss",
    "warningCount": 4,
    "errorCount": 0,
    "messages": [
        {
            "ruleId": "property-sort-order",
            "line": 2,
            "column": 3,
            "message": "Expected `height`, found `composes`",
            "severity": 1
        },
        {
            "ruleId": "property-sort-order",
            "line": 3,
            "column": 3,
            "message": "Expected `width`, found `height`",
            "severity": 1
        },
        {
            "ruleId": "property-sort-order",
            "line": 5,
            "column": 3,
            "message": "Expected `border`, found `width`",
            "severity": 1
        },
        {
            "ruleId": "property-sort-order",
            "line": 6,
            "column": 3,
            "message": "Expected `composes`, found `border`",
            "severity": 1
        }
    ]
}

though not the same.

[benthemonkey]

Writing expectations in such a way makes 0 sense.

https://github.com/sasstools/sass-lint/blob/develop/tests/rules/property-sort-order.js#L249

When a single test breaks, all the other tests break too. And you have zero clue as to what is the expectation of the error.

I made the same complaint about the tests for this project. The argument is that these tests serve as integration tests, making sure that the rules function correctly when applied to an entire stylesheet. I think they need to be paired with unit tests, which more explicitly test the output of a single ruleset. But I am lazy and haven't gotten around to adding any such tests to my rules...

[benthemonkey]

First you check if order is a string. Then you check if order is an object.

The else { block will never be reached? (what else can order be?)

A quick hop over to coveralls confirms that the "else" is never reached with our current tests.

[gajus]

I made the same complaint about the tests for this project. The argument is that these tests serve as integration tests, making sure that the rules function correctly when applied to an entire stylesheet. I think they need to be paired with unit tests, which more explicitly test the output of a single ruleset. But I am lazy and haven't gotten around to adding any such tests to my rules...

In addition, it does not make sense to have a single CSS subject to be used against multiple (9 in this specific case) tests. You want to change CSS subject to see how it affects your test (and if your changes do not break others tests) and BOOM! all tests are failing.

[DanPurdy]

Sorry @gajus I didn't write this one, I just updated it the other day for another feature request, haven't looked into it too deeply yet but was going to as part of this issue and a few other requests.

As for the tests, yeah I agree that it's not the most robust way of testing, it's helpful in most black and white cases for checking against a known target but we're missing proper unit tests in my opinion we should alse be checking error messages match our expectations etc. This is just a byproduct of the speed we've been trying to get this project up and running and the reason that I started contributing in the first place!

The plans for v2 are for a massive overhaul of absolutely everything so i would expect testing to be part of that for sure. If you have suggestions then open an issue up for these, I can't speak for the other contributors but it's always good when more and more people contribute as some of our time has been extremely limited recently and I just want to help get this project in the best place possible.

I can't comment on the errors you're seeing i'm afraid as I'm not sure what you're trying, but happy to help out if you need it once I get a bit of time later on.

[gajus]

A quick hop over to coveralls confirms that the "else" is never reached with our current tests.

I am going to make a contribution that in effect rewrites the current property-sort-order rule, including the tests and the way the errors are being reported (copying reporting style from https://github.com/brigade/scss-lint/blob/master/lib/scss_lint/linter/README.md#propertysortorder).

I understand that due to the nature of major rewrite this PR might not be accepted. In that case, I will create a repository for a plugin.

[gajus]

including the tests and the way the errors are being reported

The issue with the current way sorting errors are being reported is described here, gajus/canonical#7.

[DanPurdy]

Let's see how it goes! I think you linked to the wrong issue but I think I know the one you mean!

I made the same complaint about the tests for this project.

@benthemonkey sorry to be pedantic but I don't think 'complaint' is the right term to use here, after all everyone here is giving up their free time to work on this as are you. Let's try and stay constructive. I saw your other comment earlier too that you edited, email me or open an issue if you've got something bothering you that you wanna get sorted, be happy to help. 😄

[benthemonkey]

How about, "I've voiced the same concern"? I don't intend to sound pessimistic, and I certainly don't mean to offend anyone. I'll work on my tone. I'm super excited to be working on a project that will save developers so much time and become so popular, this earlier in its development.

That other comment (some day my "LGTM" will mean something) is simply acknowledging the fact that I'm not familiar enough with the code to put much weight behind my reviews.

[DanPurdy]

Ha we all know tone doesn't come across on the internet! 😉 I was just worried you were frustrated by something, happy to help if you are. As for your familiarity you're a contributor like us, you've written some of the more complex rules so don't feel you're any less familiar than us! Anyway, back on topic!

@gajus I'm happy to see this move forward!

[gajus]

Here is the first PR. I will try to get the separation enabling PR before Monday.

#423

[gajus]

Raised a PR that enables group separation.

#423 (comment)