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 default settings #16

Merged
merged 2 commits into from
Jun 22, 2017
Merged

Fix default settings #16

merged 2 commits into from
Jun 22, 2017

Conversation

vutran
Copy link
Contributor

@vutran vutran commented Jun 21, 2017

This PR aligns the default settings for prettier based on the current settings specified in the prettier repo.

@mitermayer
Copy link
Member

mitermayer commented Jun 22, 2017

Hi @vutran , thank for submiting a pull request!

vim-prettier default settings differ from prettier intentionally. The default settings used on vim-prettier are meant to follow the same defaults that we use internally at Facebook and on Facebook open source projects.

You can always overwrite the internal defaults by using our configuration hooks on your .vimrc.

Saying that based on your file changes I can see spliting this into 2 potential PR's would make sense:

  1. Updating our README.md and doc/prettier.txt to explain the reasoning behind our default settings choice, and being clear that we differ from prettier defaults.
  2. Add support for JSON parsing

Updating our README.md and doc/prettier.txt to explain the reasoning behind our default settings choice

We could make it more clear the original intention of the vim-prettier default settings. Please feel free to submit a PR for it or update this PR to it.

Add support for JSON parsing

I have also noticed that you mentioned the support for JSON on the README, that is awesome. But can we do that on a different PR ? To add support for JSON we would also need to do some other updates:

  1. copy ftplugin/typescript.vim to ftplugin/json.vim
  2. edit the newly created ftplugin/json.vim and change the parser from typescript to json
  3. update README.md:
    • update line " flow|babylon|typescript|postcss| to include json similar to what you did on this PR
    • update line \ 'for': ['javascript', 'typescript', 'css', 'less', 'scss'] } to include json
    • update lines autocmd BufWritePre... to include.json
  4. same updates done on README.md needs to be done ondoc/prettier.txt
  5. update package.json prettier version to the latest prettier that has support for the json parser throught the CLI
  6. Test to make sure all is working :)

Thank you very much for contributing!

@mitermayer
Copy link
Member

Also the json parser CLI setting is not yet been released to NPM, however we can have the PR ready for it.

I believe there will be a new prettier release soon that will include it.

@vutran
Copy link
Contributor Author

vutran commented Jun 22, 2017

Sounds good. I'll update this PR with the changes mentioned and another one with the JSON settings.

@vutran vutran mentioned this pull request Jun 22, 2017
2 tasks
@mitermayer
Copy link
Member

Awesome!

@mitermayer mitermayer merged commit bf82f3b into prettier:master Jun 22, 2017
@vutran vutran deleted the fix-defaults branch June 22, 2017 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants