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 custom properties sets followed by comments in no-extra-semicolons #2396

Merged
merged 3 commits into from Mar 7, 2017

Conversation

5 participants
@hudochenkov
Member

hudochenkov commented Feb 25, 2017

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

#2395

Is there anything in the PR that needs further explanation?

Just failing tests. Have no time to look into the problem yet.

@hudochenkov hudochenkov changed the title from Failing tests for no-extra-semicolons to False negative tests for no-extra-semicolons Feb 25, 2017

@hudochenkov hudochenkov force-pushed the no-extra-semicolons-failing-tests branch from 61aa6b7 to ae3a821 Feb 26, 2017

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Feb 26, 2017

Tests look good, thanks. Labelling as revision needed.

@evilebottnawi Any ideas how this can be addressed?

@jeddy3 jeddy3 changed the title from False negative tests for no-extra-semicolons to Fix custom properties sets followed by comments in no-extra-semicolons Feb 26, 2017

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Mar 1, 2017

I tried to find a solution, but failed.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 1, 2017

@hudochenkov i am champion this rule, i can see this late, it is difficult rule 😄
How about blur all comments? Maybe it is help?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 1, 2017

it is break my mind, we support custom properties sets which not even working draft, also postcss parses them very badly, i think there is always a solution we just need to more think. But i hope the next verision postcss help us and change raws for custom-* (and other stuff).

/cc @ai what we can hear about the future version and custom stuff?

@ai

This comment has been minimized.

Contributor

ai commented Mar 1, 2017

You talking about adding ; to rules raws in this cases:

--font-bodytext: {
    font-variant-ligatures: common-ligatures, contextual;
    font-variant-numeric: oldstyle-nums, proportional-nums;
    font-feature-settings: "kern", "liga", "clig", "calt", "onum", "pnum";
    font-kerning: normal;
};

I am waiting some spec stabilization and we can add it in 6.0

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 1, 2017

@ai also about sets

:root { 
  --foo: { color: red };
  --foo-bar: { color: red };
}
@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 1, 2017

We use *.toString() it is potential incorrect position in non standard syntax, need create issue and change all *.toString() to *.source.input.css in all rules before release 8.0.0. If agree create issue or give 👍 and tomorrow i create issue.

@hudochenkov

Good job!

Let's add accepted test for :root { --foo: { color: red }/* comment */; --bar: { color: blue }; }. Comment before ;.

const prev = node.prev()
if (isCustomPropertySet(node) && node.parent.index(node) > 0 && prev && prev.type === "comment") {
rawBeforeNode = prev.toString() + rawBeforeNode

This comment has been minimized.

@hudochenkov

hudochenkov Mar 1, 2017

Member

Should it be prev.source.input.css? :)

This comment has been minimized.

@evilebottnawi

evilebottnawi Mar 2, 2017

Member

@hudochenkov i do this in another PR 😄 #2396 (comment)

if ((isCustomPropertySet(node) && node.parent.index(node) > 0)
|| (node.type === "comment" && next
&& (isCustomPropertySet(next) && node.parent.index(next) > 0 || next.type === "comment"))
) {

This comment has been minimized.

@hudochenkov

hudochenkov Mar 1, 2017

Member

What do you think about splitting this if statement into else if or more if statements?

This comment has been minimized.

@evilebottnawi

evilebottnawi Mar 2, 2017

Member

@hudochenkov it's possible, but why 😄

This comment has been minimized.

@hudochenkov

hudochenkov Mar 2, 2017

Member

Too complicated. Hard to understand in current form.

@evilebottnawi evilebottnawi force-pushed the no-extra-semicolons-failing-tests branch from 8e7b340 to 420820c Mar 2, 2017

@evilebottnawi evilebottnawi force-pushed the no-extra-semicolons-failing-tests branch from 420820c to c195085 Mar 2, 2017

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Mar 2, 2017

/cc @hudochenkov done

@@ -60,7 +60,7 @@ const rule = function (actual) {
let allowedSemi = 0
// Forbid semicolon before first custom properties sets
if ((isCustomPropertySet(node) && node.parent.index(node) > 0)) {
if (isCustomPropertySet(node) && node.parent.index(node) > 0) {

This comment has been minimized.

@evilebottnawi

evilebottnawi Mar 2, 2017

Member

Thanks 😄

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Mar 2, 2017

LGTM! Thanks!

@davidtheclark davidtheclark merged commit 329d0aa into master Mar 7, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@davidtheclark davidtheclark deleted the no-extra-semicolons-failing-tests branch Mar 7, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Mar 7, 2017

Added to Changelog:

  • Fixed: no-extra-semicolons false positives for comments are custom property sets (#2396).

m-allanson added a commit that referenced this pull request Mar 12, 2017

Merge branch 'master' into v8
* master:
  Update CHANGELOG.md
  Add flow comments annotation to function signatures (#2319)
  Update CHANGELOG.md
  Fix custom properties sets followed by comments in no-extra-semicolons (#2396)
  Fixed: `value-keyword-case` is now ignore counter, counters and attr fucntions.
  Fix typos (#2412)
  Fixed: `value-keyword-case` is now ignore counter-reset property.
  chore(package): update flow-bin to version 0.41.0
  chore(package): update eslint to version 3.17.0
  Fix the example for Custom Message in docs (#2409)
  Update CHANGELOG.md
  Update CHANGELOG.md
  Add at-rule-semicolon-space-before rule (#2388)
  Update CHANGELOG.md
  Print out filenames/globs when files do not exist (#2328)
  Tests: improved tests for `no-missing-end-of-source-newline` rule. (#2384)
  Update stylelint-disable-reason changelog entry (#2393)
  Add min rules
  chore(package): update flow-bin to version 0.40.0 (#2390)
  chore: Update `remark-*` modules (#2391)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment