-
-
Notifications
You must be signed in to change notification settings - Fork 930
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
Deprecate rules #2197
Deprecate rules #2197
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m-allanson Fantastic! Thanks!
Code looks good to me. I don't have time for another little while to double-check the output in detail — can someone else on @stylelint/core review and merge?
|
||
```css | ||
a { color: red; } | ||
/** ↑ ↑ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra arrow.
|
||
```css | ||
a { color: red; } | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of three separate code blocks, I suggest one block following "But not these patterns:" No need for extra verbosity, I think.
a { color: red; } | ||
``` | ||
|
||
To allow single line blocks, use the `"always-multi-line"` option for both rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about: To allow single-line blocks but enforce newlines with multi-line blocks, ...
@@ -17,6 +17,8 @@ a { /* end-of-line comment */ | |||
} | |||
``` | |||
|
|||
Refer to [the FAQ](../../../docs/user-guide/faq.md#how-do-i-disallow-single-line-blocks) for more information on using this rule with [`block-opening-brace-newline-before`](../block-opening-brace-newline-before/README.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about appending "to disallow single-line rules."
@@ -9,6 +9,8 @@ Require a newline or disallow whitespace before the opening brace of blocks. | |||
* The newline before this brace */ | |||
``` | |||
|
|||
Refer to [the FAQ](../../../docs/user-guide/faq.md#how-do-i-disallow-single-line-blocks) for more information on using this rule with [`block-opening-brace-newline-after`](../block-opening-brace-newline-after/README.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about appending "to disallow single-line rules."
const results = data.results[0] | ||
expect(results.deprecations.length).toEqual(1) | ||
expect(results.deprecations).toEqual(deprecationResult) | ||
expect(results.invalidOptionWarnings.length).toBe(0) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any of the tests in this particular file that tested declaration-block-properties-order
should just be removed, I think. We don't want validate-options
tests to be tangled up with deprecations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'll create a separate file for the deprecation test.
@@ -1,7 +1,16 @@ | |||
exports[`test 002 1`] = ` | |||
Array [ | |||
Object { | |||
"deprecations": Array [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea to add these system tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The system tests are great for changes like this 👍 .
I can review and merge tomorrow. |
@davidtheclark Good catches, thanks! I've updated the PR to account for your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation code instead looks good to me.
I've made a request that we make use of the release planning doc.
@@ -1,5 +1,7 @@ | |||
# declaration-block-no-ignored-properties | |||
|
|||
***Deprecated: this rule is outside the scope of stylelint's functionality. See [this comment](https://github.com/stylelint/stylelint/issues/2047#issuecomment-261382105) for details.*** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's point to http://stylelint.io/user-guide/release-planning/ here instead (and, in a separate PR, add something there about each of these deprecated rules)
This provides us with a single destination for 8.0.0
changes and planning, where we can create a consistent narrative that we can link to when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hang on, I've muddled the branches a bit. Will fix it soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed.
@jeddy3 this is ready for another look. |
@m-allanson Awesome thanks! I trust you're having a pleasant xmas? I'm going to try to catch-up on a few stylelint issues and PRs today... I see you've been busy pushing code :) |
@jeddy3 Very much so, it's been surprisingly calm around here :) |
Apart from the awesome PR's you've been submitting @m-allanson 🎉 |
* Deprecate rules * Add system test for deprecated rules * Update docs to account for deprecated rules * Documentation tweaks * Use a separate file to test the deprecation for declaration-block-properties-order * Point to release planning docs in deprecated rule READMEs
#2123
Merges into a new
deprecations
branch.