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 support for stdin input #2787

Merged
merged 2 commits into from
Oct 30, 2017
Merged

Add autofix support for stdin input #2787

merged 2 commits into from
Oct 30, 2017

Conversation

mahmost
Copy link
Contributor

@mahmost mahmost commented Jul 30, 2017

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

#2710

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@alexander-akait
Copy link
Member

@mahmost Thanks!

@mahmost
Copy link
Contributor Author

mahmost commented Aug 9, 2017

Since maintainers seem busy at the moment .. I'm willing to add some more information to help reviewing the PR

Here is what was done :

  • In case --fix is used with stdin input, the output (to stdout) is the fixed style code
  • One simple test was added to autofix related tests ensuring that when autofixing stdin code, result.output contains the fixed style code

Other additions and consideration that was NOT done in this PR :

  • Outputting the normal output (linting rules) to stderr (this may even be an option --also-output-rules or anything like that)
  • More tests (I just looked at the other autofix tests and added what I thought to be enough for now)

P.S. Closing the PR was by mistake (sorry for that)

@mahmost mahmost closed this Aug 9, 2017
@mahmost mahmost reopened this Aug 9, 2017
return prepareReturnValue([stylelintResult]);
const returnValue = prepareReturnValue([stylelintResult]);
const postcssResult = stylelintResult._postcssResult;
// If we're fixing the output should be the fixed code
Copy link
Member

Choose a reason for hiding this comment

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

// If we're fixing, the output should be the fixed code the added comma here helps readability

@ntwb
Copy link
Member

ntwb commented Aug 12, 2017

Thanks @mahmost, see the above for one change to the code comment and I'd like to see a few more tests added if you could please?

I'm thinking in particular a test or two thats tests the output formatting when {, }, \r, \n are used with some of the new line before/after opening/closing braces rules

@ghost
Copy link

ghost commented Oct 9, 2017

Good day
Guys, any progress here?
Maybe i can help with final stage of this pr?

@alexander-akait
Copy link
Member

@mahmost friendly ping

@modosc
Copy link
Member

modosc commented Oct 12, 2017

i'm working on this now.

@modosc
Copy link
Member

modosc commented Oct 12, 2017

I'm thinking in particular a test or two thats tests the output formatting when {, }, \r, \n are used with some of the new line before/after opening/closing braces rules

@ntwb afaict there aren't --fix rules for any of those - they're still on the list in #2829. if we add the comma and rebase is that enough?

@ghost
Copy link

ghost commented Oct 12, 2017

@mahmost Thanks!

@modosc
Copy link
Member

modosc commented Oct 28, 2017

@stylelint/core ping - sorry for the wide net, can anyone respond to the above comments? would love to get this going forward.

@CAYdenberg
Copy link
Contributor

As the autofixed rules are minimal for now and none of them affect things like line breaks, I recommend we merge with the single test and address any issues that crop up moving forward. Any thoughts @ntwb ?

@modosc With stale PRs (and simple changes) I usually make them myself and push to the contributors branch (if possible) or open a new PR. In this case would you like to do the honours?

@mahmost
Copy link
Contributor Author

mahmost commented Oct 29, 2017

Sorry for not giving this enough time, this commit addresses the requested comment change, and handles ignored files (by not returning anything if filename supplied via --stdin-filename is ignored)
Still, the issues of new lines and spaces seem to persist for now

@modosc
Copy link
Member

modosc commented Oct 30, 2017

As the autofixed rules are minimal for now and none of them affect things like line breaks, I recommend we merge with the single test and address any issues that crop up moving forward. Any thoughts @ntwb ?

let's go ahead with this and once we start fixing newlines we can add appropriate tests (i understand the concern around the output in that case for sure).

@modosc modosc merged commit ac7c141 into stylelint:master Oct 30, 2017
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

6 participants