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 media-feature-range-operator-space-before #3618

Merged
merged 2 commits into from Sep 6, 2018

Conversation

4 participants
@ota-meshi
Member

ota-meshi commented Aug 26, 2018

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

closes #3580

Is there anything in the PR that needs further explanation?

if use the --fix option, to remove whitespace at the end of each line.

ex.

  • option: always

    code:

    @media (width<600px) {}

    fixed:

    @media (width <600px) {}
  • option: never

    code:

    @media (width < 600px) {}

    fixed:

    @media (width< 600px) {}
result,
ruleName
if (fixOperatorIndices.length) {
let params = atRule.raws.params

This comment has been minimized.

@boccob

boccob Aug 27, 2018

Contributor

As far as I can see there is no params in raws: http://api.postcss.org/Node.html#raws

This comment has been minimized.

@ota-meshi

This comment has been minimized.

@boccob

boccob Aug 27, 2018

Contributor

Is there any difference between getting / setting node.params and node.raws.params.raw?
If there is no difference I think it'd be better to use documented parameters.

This comment has been minimized.

@ota-meshi

ota-meshi Aug 27, 2018

Member

node.raws.params.raw to be create if comments get in. In this case, node.params will not contain comment data.
I tried finding but I do not know the existence of the document yet. . .

This comment has been minimized.

@boccob

boccob Aug 27, 2018

Contributor

Got it. Thank you!

? atRule.raws.params.raw
: atRule.params;
fixOperatorIndices
.sort((a, b) => a - b)

This comment has been minimized.

@boccob

boccob Aug 27, 2018

Contributor

I'd replace .sort((a, b) => a - b).reverse() with .sort((a, b) => b - a).

@@ -26,26 +26,54 @@ const rule = function(expectation) {
}
root.walkAtRules(/^media$/i, atRule => {
const fixOperatorIndices = [];
findMediaOperator(atRule, checkBeforeOperator);

This comment has been minimized.

@boccob

boccob Aug 27, 2018

Contributor

The whole process is not very obvious. You declare array -> call findMediaOperator with callback that could be executed several times and that mutates array of indices.
Maybe instead of this you can create a new function that will fix spacing. And you will run this function from checkBeforeOperator.
What do you think?

This comment has been minimized.

@ota-meshi

ota-meshi Aug 27, 2018

Member

I think that is good. I will fix it the next time I can work.

@ota-meshi

This comment has been minimized.

Member

ota-meshi commented Aug 27, 2018

@boccob
I changed the content pointed by your review.

@boccob

boccob approved these changes Sep 2, 2018

@jeddy3

jeddy3 approved these changes Sep 6, 2018

@ntwb

ntwb approved these changes Sep 6, 2018

@jeddy3 jeddy3 merged commit 9c133b2 into master Sep 6, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 96.222%
Details

@jeddy3 jeddy3 deleted the add-autofix/media-feature-range-operator-space-before branch Sep 6, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented Sep 6, 2018

  • Added: media-feature-range-operator-space-before autofix (#3618).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment