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

Add autofix to function-calc-no-unspaced-operator #5273

Conversation

doing-art
Copy link
Contributor

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

This PR contains implementation of the autofix for the function-calc-no-unspaced-operator rule as it is required in #4866.

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

@doing-art
Copy link
Contributor Author

Hi all
Could somebody please trigger a rebuild on the CI build? One check failed and it seems to be happened because of some infrastructural issue.
Thank you

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[suggestion] How about adding a new utility isNewlineAtIndex() as below?

lib/rules/function-calc-no-unspaced-operator/index.js Outdated Show resolved Hide resolved
lib/rules/function-calc-no-unspaced-operator/index.js Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

[suggestion] How about using more generic function names and unifying the use of String.prototype.slice() instead of substr() or substring()?

IMO, substr() and substring() seem confusing, so I recommend using slice() instead.

See also:

lib/rules/function-calc-no-unspaced-operator/index.js Outdated Show resolved Hide resolved
lib/rules/function-calc-no-unspaced-operator/index.js Outdated Show resolved Hide resolved
@ybiquitous
Copy link
Member

@doing-art Thank you for creating this change! 👍🏼

The logic seems good to me, but I've left some comments to improve the code readability and maintainability.

@jeddy3 jeddy3 changed the title Feature/4866 add autofix to function calc no unspaced operator Add autofix to function-calc-no-unspaced-operator May 11, 2021
doing-art and others added 5 commits May 16, 2021 20:58
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@doing-art
Copy link
Contributor Author

@doing-art Thank you for creating this change! 👍🏼

The logic seems good to me, but I've left some comments to improve the code readability and maintainability.

Thank you for reviewing my PR and sharing knowledge about String methods!
I completely agree with all your comments. The PR is updated as you have suggested. Please take a look at it when you get a chance.

@ybiquitous
Copy link
Member

@doing-art Thank you! It seems good to me. 👍🏼

By the way, we are now working on the next version 14 (#5205), which includes many significant changes such as ESM, PostCSS 8, etc.
So, I think it seems good to change this PR's base branch from mater to v14.

What do you think? @jeddy3

@jeddy3
Copy link
Member

jeddy3 commented May 17, 2021

So, I think it seems good to change this PR's base branch from mater to v14.

I agree. We'll want to minimise conflicts and save ourselves a headache by working from the v14 branch.

@doing-art Can you change the merge branch, please?

@doing-art doing-art changed the base branch from master to v14 May 17, 2021 12:17
@doing-art
Copy link
Contributor Author

@jeddy3 @ybiquitous Changed the merge branch and resolved one small conflict in the tests.

@jeddy3
Copy link
Member

jeddy3 commented May 17, 2021

Changed the merge branch and resolved one small conflict in the tests.

Thanks!

You'll need to run npm run format to fix formatting. Tests are looking good, though.

@doing-art
Copy link
Contributor Author

Changed the merge branch and resolved one small conflict in the tests.

Thanks!

You'll need to run npm run format to fix formatting. Tests are looking good, though.

Sorry, my bad. Have fixed linting issue.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@doing-art Thank you! Looks good to me. 👍🏼

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jeddy3 jeddy3 merged commit 3d5725e into stylelint:v14 May 17, 2021
@jeddy3
Copy link
Member

jeddy3 commented May 17, 2021

v14 changelog updated:

  • Added: function-calc-no-unspaced-operator autofix (#5273).

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.

None yet

3 participants