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

feat: initial support for prettier-stylelint #218

Merged
merged 10 commits into from
Oct 2, 2017

Conversation

hugomrdias
Copy link
Contributor

this adds initial support for prettier-stylelint, im still adding features but the basic stuff at least for the extension works well.

please keep in mind that i don't do ts or vscode extensions much :) be gentle

ref #207

@CiGit
Copy link
Member

CiGit commented Sep 18, 2017

Thanks for that PR and your prettier-stylelint.
Could your prettier-stylint format be synchronous ? To be able to use safeExecution without having to export everything ?

@hugomrdias
Copy link
Contributor Author

stylelint doesnt have a sync api, i can create a safeExecutionAsync instead and leave everything abstracted in the utils

@CiGit
Copy link
Member

CiGit commented Sep 19, 2017

This is also a solution. Make safeExecution handle both.

@hugomrdias
Copy link
Contributor Author

@CiGit does this seem good to you ?

@CiGit
Copy link
Member

CiGit commented Sep 24, 2017

Yup, I prefer. asking @RobinMalfait for a review of safeExecution

About prettier-stylelint, which prettier/ stylelint version does it pickup?

Copy link
Contributor

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

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

Code wise LGTM, except for the one little nitpick.

addToOutput(addFilePath(err.message, fileName));
updateStatusBar('Prettier: $(x)');

console.log(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to remove this line

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, now that I'm thinking about it, the safeExecution has a lot of duplicated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ofc damn .. I'll remove that

@hugomrdias
Copy link
Contributor Author

@CiGit for now the bundled version, but I'll be fixing that in a near future

@hugomrdias
Copy link
Contributor Author

@RobinMalfait pushed a commit removing the console log

@CiGit
Copy link
Member

CiGit commented Sep 25, 2017

My concerns are how often do you expect it to be updated, until it has all the feature you'd like ?
On your repo I see 9 releases in 8 days. I don't think we will be able to follow so many releases here :-)

@hugomrdias
Copy link
Contributor Author

hugomrdias commented Sep 25, 2017

i dont expect to have that many releases from now on. besides ill keep sending PR's here for each new release.
plus after i add support for relative package lookup (which im already working on) this extension doesn't need to have the latest version bundled to work properly

@hugomrdias
Copy link
Contributor Author

@CiGit with this last commit your concerns should be addressed

@CiGit
Copy link
Member

CiGit commented Sep 30, 2017

Please resolve conflicts, I'll merge after.

@CiGit
Copy link
Member

CiGit commented Sep 30, 2017

And update the readme please :-)

@CiGit CiGit merged commit 6e46c4c into prettier:master Oct 2, 2017
@CiGit
Copy link
Member

CiGit commented Oct 2, 2017

Thanks !

@hugomrdias hugomrdias deleted the feat/support-prettier-stylelint branch October 2, 2017 18:01
@WickyNilliams
Copy link

Is there some way to configure this to run over JS files? Use case: styled-components.

The regular stylelint extension for vscode has a config option additionalDocumentSelectors so you can configure it to run over JS files as well as the usuall css/scss/less. eg:

  "stylelint.additionalDocumentSelectors": [
    "javascriptreact",
    "javascript"
  ]

Happy to make an issue in the appropriate repo, but thought i'd ask here given this was the integration point

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

Successfully merging this pull request may close these issues.

4 participants