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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of shared-line comments #2262

Merged
merged 4 commits into from Jan 21, 2017

Conversation

4 participants
@davidtheclark
Contributor

davidtheclark commented Jan 15, 2017

Introduces a number of utilities and adjusts the following rules: at-rule-empty-line-before, custom-property-empty-line-before, declaration-empty-line-before, rule-nested-empty-line-before, rule-non-nested-empty-line-before.

Closes #2237; closes #2239; closes #2240.

This turned out to be a lot of work 馃槄

cc @jeddy3 @hudochenkov

Fix handling of shared-line comments
Introduces a number of utilities and adjusts the following rules:
at-rule-empty-line-before, custom-property-empty-line-before,
declaration-empty-line-before, rule-nested-empty-line-before,
rule-non-nested-empty-line-before

Closes #2237; closes #2239; closes #2240.
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jan 16, 2017

This turned out to be a lot of work 馃槄

I bet! It's looking great though.

However, (and I don't think you're going to like this as it might mean there's a little more legwork to do), there could be a couple of conflicts down the line if we merge this now. In #2213 we standardised a couple of options. This PR was merged into the deprecations branch. We're waiting on #2123 before merging into master (so that all the deprecations go out in one go). There is now nothing blocking #2123 btw as the deprecation list has been finalised with the closure of #2126.

I think only a small fraction of the PR is going to conflict; perhaps just the doc changes and first-nested stuff.

What do you think is the best way to go about merging? Originally, I thought deprecations into master, then rebase v8 on master, then make these changes.

Can we merge this PR into v8 now, and the do deprecations after? Or is that going to messy?

@hudochenkov

Fantastic job! You did much better than I think would do :)

I especially like how you did isSharedLineComment and getPreviousNonSharedLineCommentNode. I didn't think of shared-line comment before node. You have much broader view on possible code styles.

Also this PR closes #2212.

expectEmptyLineBefore = !expectEmptyLineBefore
}
// Optionally reverse the expectation if a rule precedes this node
if (optionsMatches(opts.options, "except", "after-rule") && opts.rule.prev() && opts.rule.prev().type === "rule") {
if (optionsMatches(opts.options, "except", "after-rule") && _.get(getPreviousNonSharedLineCommentNode(opts.rule), "type") === "rule") {

This comment has been minimized.

@hudochenkov

hudochenkov Jan 16, 2017

Member

Do you use _.get(getPreviousNonSharedLineCommentNode(opts.rule), "type") === "rule") instead of getPreviousNonSharedLineCommentNode(opts.rule) && getPreviousNonSharedLineCommentNode(opts.rule).type === "rule" to make code shorter and more readable?

This comment has been minimized.

@davidtheclark

davidtheclark Jan 16, 2017

Contributor

Yep, I find it more concise and readable.

expect(isSharedLineComment(root.nodes[0])).toBe(true)
})
it("returns true for a shared-line comment before a rule", () => {

This comment has been minimized.

@hudochenkov

hudochenkov Jan 16, 2017

Member

Duplicate test. Same as previous statement.

This comment has been minimized.

@davidtheclark

davidtheclark Jan 16, 2017

Contributor

Thanks for checking so carefully!

"use strict"
module.exports = function (statement/*: postcss$rule | postcss$atRule*/)/*: boolean*/ {
if (statement.type !== "rule" && statement.type !== "atrule") {

This comment has been minimized.

@hudochenkov

hudochenkov Jan 16, 2017

Member

Not only at-rule-empty-line-before, rule-nested-empty-line-before have first-nested option. comment-empty-line-before, custom-property-empty-line-before, declaration-empty-line-before have it also. This helper can be used for more rules.

return _.get(node, "source.start.line")
}
module.exports = function getNextNonSharedLineCommentNode(node/*:: ?: postcss$node*/)/*: postcss$node | void*/ {

This comment has been minimized.

@hudochenkov

hudochenkov Jan 16, 2017

Member

I don't know if it matters, but getPreviousNonSharedLineCommentNode has slightly different formatting for this line.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jan 16, 2017

What do you think is the best way to go about merging?

@jeddy3 I say we merge now, fix conflicts as they arise. Fixing conflicts is always messy but inevitable 炉_(銉)_/炉

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jan 16, 2017

@hudochenkov Thanks for the feedback! I made the relevant fixes.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 19, 2017

@davidtheclark please, take a look at comment-empty-line-before. You can apply new helpers there.

Update: I was looking at the first commit in this PR, and now I see you make some changes in comment-empty-line-before.

As I know between-comments was renamed to after-comment in deprecations branch. Probably you can apply isAfterCommentLine for that option.

@hudochenkov

Do you think this part in comment-empty-line-before can be improved?

return false
}
let previousNode = atRule.prev()

This comment has been minimized.

@hudochenkov

hudochenkov Jan 19, 2017

Member

What about using here getPreviousNonSharedLineCommentNode() instead?

This comment has been minimized.

@davidtheclark

davidtheclark Jan 19, 2017

Contributor

Yeah 鈥 there are probably a million places this new utility could be used.

return false
}
let previousNode = atRule.prev()

This comment has been minimized.

@hudochenkov

hudochenkov Jan 19, 2017

Member

What about using here getPreviousNonSharedLineCommentNode() instead?

previousNode = previousNode.prev()
}
return _.get(previousNode, "name") == atRule.name

This comment has been minimized.

@hudochenkov

hudochenkov Jan 19, 2017

Member

Shall we use === instead?

This comment has been minimized.

@evilebottnawi

This comment has been minimized.

@davidtheclark

davidtheclark Jan 19, 2017

Contributor

Yes, this was an oversight.

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jan 20, 2017

@hudochenkov Addressed the latest few comments. Good ones. Thanks again.

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Jan 20, 2017

@davidtheclark I hope you don't mind I've made some changes to comment-empty-line-before. #courage

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jan 20, 2017

@hudochenkov 馃憤 looks great, thanks. I forgot about that bit of feedback at least once, so you did the right thing.

@davidtheclark davidtheclark merged commit d436f0b into v8 Jan 21, 2017

2 checks passed

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 2240-fix branch Jan 21, 2017

@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Jan 21, 2017

Closed by #2262.

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Jan 21, 2017

Awesome job!

jeddy3 added a commit that referenced this pull request Jul 14, 2017

8.0.0 (#2735)
* Add stylelint semantic versioning policy

* Fix whitespace and empty lines

* Add changelog entry

* Remove stylelint-config=standard reference

* Tweak major release optional options verbiage

* Update changelog

* Props ESLint

* Fix false negatives with ignore: ["descendant"] in selector-no-type (#2200)

* Update CHANGELOG.md

* Fix handling of shared-line comments (#2262)

* Fix handling of shared-line comments

Introduces a number of utilities and adjusts the following rules:
at-rule-empty-line-before, custom-property-empty-line-before,
declaration-empty-line-before, rule-nested-empty-line-before,
rule-non-nested-empty-line-before

Closes #2237; closes #2239; closes #2240.

* Fixes based on feedback

* Use getPreviousNonSharedLineCommentNode more

* Use more new helpers in comment-empty-line-before

* Add ignore: ["child"] option to selector-no-type (#2217)

* Add ignore: ["child"] option to selector-no-type

* Correct logic for ignore: ["child"] option

* Include line and column in reject tests with multiple type selectors

* Update CHANGELOG.md

* Report every selector resolved selector in selector-max-compound-selectors (#2350)

* Update CHANGELOG.md

* Add new shorthand data (#2354)

* Add new shorthand data

* Add mask shorthand

* Use defined list of properties in shorthand-property-no-redundant-values

* Improve declaration-block-no-shorthand-property-overrides speed a little bit

* Check prefixed properties agains prefixed properties in declaration-block-no-shorthand-property-overrides

* Improve declaration-block-no-redundant-longhand-properties speed a little bit

* Ignore prefixes case in declaration-block-no-shorthand-property-overrides

* Check prefixed properties agains prefixed properties in declaration-block-no-redundant-longhand-properties

* Add `grid-gap` to shorthand-property-no-redundant-values

* Explain properties set for shorthand-property-no-redundant-values

* Revert unnecessary changes

* Add 鈥渕ask鈥 to declaration-block-no-redundant-longhand-properties readme

* Update CHANGELOG.md

* Update changelog

* Remove deprecated rules (#2422)

* Remove deprecated rules

* Include `rule-empty-line-before` in system tests

* *-empty-line-before rules consider line as empty if it contains whitespace only (#2440)

* Update CHANGELOG.md

* Remove deprecated options (#2433)

* Update CHANGELOG.md

* report-needless-disables now exits with non-zero code (#2341)

* Update CHANGELOG.md

* Remove leftover files (#2442)

* Remove leftover file

* One more

* And one more

* Ignoring improvements (#2464)

* WIP ignoring improvements

* Remove standaloneIgnored property of result

* Add disableDefaultIgnores

* Add documentation tweaks

* Add parseStylelintIgnore tests

* Non-absolutized ignore file paths

* Restire gitignore syntax

* Remove parseStylelintIgnore

* Update CHANGELOG.md

* Check all linear-gradients in a value list (#2496)

Closes #2481

* Update CHANGELOG.md

* Update CHANGELOG.md

* Respect line case for `ignorePattern` (#2683)

Closes #2647

* Update CHANGELOG.md

* Update decls

* Fix remark warning

* Update snapshots

* Add missing message to reject test

* Fix tests for *-empty-line-before rules (#2688)

* Fix tests for *-empty-line-before rules

* Test addition and tweak

* Update to PostCSS@6 (#2561)

* Update to PostCSS@6

* Fix PostCSS@6 closing-brace test (#2689)

* Account for PostCSS@6's support for at-apply

* Remove only

* Fix postcssPlugin tests (#2690)

* Fix defaultSeverity.test.js test

* Fix ignoreDisables test

* Check ownSemiColon

* Fixed: remove hacks related to custom property set.

* Remove deprecated rules (#2693)

Closes #2521

* Update CHANGELOG.md

* Update CHANGELOG.md

* Add VISION.md (#2704)

* Add releases.md (#2705)

* Add releases.md

Closes #2700

* Add dry-release step

* Use HTTPS for links

* Change ignore options for selector-max-type (#2701)

* Fix false negatives with ignore descendant

* Add ignore child

Closes #2699

* Update CHANGELOG.md

* Update README.md to account for VISION.md (#2727)

* Update README.md to account for VISION.md

* Fix lint warnings in CHANGELOG

* Update postcss-less to 1.0 (#2702)

* Update postcss-less to 1.0.2

* Update isStandardSyntaxRule for postcss-less 1.0

* Pass custom postcss stringifier when creating result

* Correct no-extra-semicolons Less test position

* Ignore Less mixins in no-extra-semicolons

* Update postcss-less to 1.1.0

* Correct no-extra-semicolons Less test position again

* Utilize isStandardSyntaxRule when checking for Less rules in no-extra-semicolons

* Case sensitive white/blacklists and ignore* options (#2709)

Closes #2660

* Update CHANGELOG.md

* docs: Add "New Release Issue Template" to releases.md (#2728)

* Exit with non-zero code on postcss warnings (#2713)

* Enhancement: exit with non zero code on postcss warnings.

* Account for parseErrors in stringformatqer

* Guard against no parseErrors

* Update CHANGELOG.md

* Refactor: simplify ignore logic. (#2732)

* Update changelog

* Bump 8.0.0 changes to top

* Correctly place changelog items

* Add introduction

* Add codefence for website

* Use relative path

* Include VISION in package for website

* Add link to VISION doc for website

* Changelog amends

* Update releases.md (#2738)

* Update eslint and config (#2737)

* Order requires

* Use eqeqeq eslint rule

* Update eslint

* Disable no-useless-escape just for this repo

* Enable no-useless-escape rule

* Use tilde

* Fix enable comment

* Add reject test for unescaped regex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment