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

fix no-unit-unknown rule for variable usage #4163

Merged
merged 1 commit into from Jul 23, 2019

Conversation

laysent
Copy link
Contributor

@laysent laysent commented Jul 22, 2019

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

Closes #4162

Is there anything in the PR that needs further explanation?

I noticed that lint-staged changed the way how it's configured. From their document, they encourage to use ignore configuration of each linter. And since this file is already in .eslintignore and .prettierignore, I believe it's safe to remote corresponding line in lint-staged config.

@hudochenkov
Copy link
Member

I wonder why it wasn't catched by !isStandardSyntaxValue(value) function? I think we need to fix this function instead of proposed fix.

@alexander-akait
Copy link
Member

@laysent
Copy link
Contributor Author

laysent commented Jul 22, 2019

I wonder why it wasn't catched by !isStandardSyntaxValue(value) function? I think we need to fix this function instead of proposed fix.

blurInterpolation removes @ and thus it won't be recognized correctly by isStandardSyntaxValue here.

I have changed the way how this issue is fixed, i.e. use node.value instead of transformed value in isStandardSyntaxValue. Please kindly have a look.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution!

Could remove lint-stages changes, please? We squash commits on merge and this change would be confusing in unrelated commit.

@laysent
Copy link
Contributor Author

laysent commented Jul 23, 2019

Nice solution!

Could remove lint-stages changes, please? We squash commits on merge and this change would be confusing in unrelated commit.

Sure, I have removed lint-stages changes.

Would you like me to send another PR for this lint-staged changes? Seems at least on my side, lint-staged won't work without changing the config in package.json.

@hudochenkov
Copy link
Member

Would you like me to send another PR for this lint-staged changes?

Yes, please :)

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@hudochenkov hudochenkov merged commit 31076ba into stylelint:master Jul 23, 2019
@hudochenkov
Copy link
Member

  • Fixed: no-unit-unknown false positives for at-variables (Less) starting with numbers (#4163).

@@ -25,7 +25,7 @@ module.exports = function(node /*: Object*/) /*: ?string*/ {

if (
node.type !== "word" ||
!isStandardSyntaxValue(value) ||
!isStandardSyntaxValue(node.value) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @hudochenkov it is break other case, for example .selector { property: 20px\9; } - regression

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's a hack for IE8.

All tests were passed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hudochenkov yes, based on code test should be failed 😕 https://github.com/laysent/stylelint/blob/0c68261c43123df7e0ffaabbdc350a8a480fb43a/lib/utils/getUnitFromValueNode.js#L29, maybe postcss-value-parser recognize this as other type for node, i will look on this today

@laysent laysent mentioned this pull request Jul 23, 2019
@laysent
Copy link
Contributor Author

laysent commented Jul 25, 2019

@hudochenkov Hi, just curious when would be the next release of stylelint? With this fixed, I could introduce the tool to our team and use it in our projects. ;-)

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

Successfully merging this pull request may close these issues.

no-unit-unknown should ignore less variable
3 participants