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(config): support TOML configuration files #4877

Merged
merged 14 commits into from Aug 3, 2018

Conversation

@jorgegonzalez
Copy link
Contributor

commented Jul 23, 2018

Closes #4845

In order to support .toml configuration files, cosmiconfig must first be updated to v5.0. This update seems to break a bunch of tests. The following errors are thrown all over the place:

image

image

Update 29/7/2018: This PR adds support for .prettierrc.toml config files.

@jorgegonzalez jorgegonzalez changed the title Add support for TOML in configuration files (.prettierrc.toml) [WIP] Add support for TOML in configuration files (.prettierrc.toml) Jul 23, 2018

@jorgegonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2018

Also, I recognize adding another dependency is not ideal for what is essentially a minor feature, but toml is quite light.

@foray1010

This comment has been minimized.

Copy link

commented Jul 29, 2018

@jorgegonzalez how about @iarna/toml? it is actively maintained, and it is the only js package support toml 0.5.0 at the time
https://github.com/iarna/iarna-toml

@j-f1

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

It’s also much smaller slightly bigger than toml when bundled.

(packagephobia tells you how big a package is when installed on your computer, while bundlephobia tells you how big it is when bundled+minified(+gzipped).)

@jorgegonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2018

@foray1010 I'll use @iarna/toml if @j-f1 will allow the size increase 🙏

@jorgegonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2018

Thanks @ikatyang for upgrading cosmiconfig!

@jorgegonzalez jorgegonzalez changed the title [WIP] Add support for TOML in configuration files (.prettierrc.toml) feat: support .prettierrc.toml configuration files Jul 29, 2018

@jorgegonzalez jorgegonzalez changed the title feat: support .prettierrc.toml configuration files feat(config): support .prettierrc.toml configuration files Jul 29, 2018

@jorgegonzalez jorgegonzalez changed the title feat(config): support .prettierrc.toml configuration files feat(config): support TOML configuration files Jul 29, 2018

@j-f1

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

Should be fine 👍

@jorgegonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2018

Not sure why it's failing on node4 🤔

@jorgegonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2018

Ah I see.

@j-f1

This comment has been minimized.

Copy link
Member

commented Jul 29, 2018

I think you could use a snapshot in that test, which would save having to create an object to match against.

`;

exports[`TOML throws error on incorrect toml 1`] = `
"TOML Error in load-toml.js:

This comment has been minimized.

Copy link
@ikatyang

ikatyang Jul 30, 2018

Member

The currentFile looks weird to me, it confused me why there's a TOML parsing error for a js file. We should rename it to something ends with .toml.

@@ -0,0 +1,12 @@
"use strict";

const toml = require("@iarna/toml");

This comment has been minimized.

Copy link
@ikatyang

ikatyang Jul 30, 2018

Member

We could use @iarna/toml/parse-string to further reduce the bundle size.

@ikatyang

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

I'll let other maintainers to decide if we should merge it as I hesitate about adding another config format. If someone wants to add support for other format later, what should we do?

I'm thinking of if we should make loaders be part of plugins so that people can use whatever format they want.

@jorgegonzalez

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2018

I understand the hesitance to add another format, and this is indeed an opinionated PR. Personally, I would love to see the JavaScript community embrace TOML configuration files more, as it is subjectively superior to both YAML and JSON configurations, and Prettier has the opportunity to lead the community in this. I suspect more users would also lean toward this format if they were given the chance to try it.

As linked in #4845:

Some reasons why TOML is better than YAML for configuration files: https://arp242.net/weblog/yaml_probably_not_so_great_after_all.html

Some arguments against JSON as a configuration language: https://www.lucidchart.com/techblog/2018/07/16/why-json-isnt-a-good-configuration-language/

@j-f1
j-f1 approved these changes Jul 31, 2018
Copy link
Member

left a comment

I think this is a great idea! It seems fairly lightweight and it shouldn’t take too much maintenance, especially if it’s integrated into cosmiconfig directly. All that’s missing now is support for printing it 😉

@suchipi

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

There's not a huge value-add imo since prettier config files are very small and you're unlikely to need comments in them, but since the work is already done let's merge it 👍

@suchipi
suchipi approved these changes Aug 3, 2018

@j-f1 j-f1 merged commit 7d78ce6 into prettier:master Aug 3, 2018

10 checks passed

ci/circleci: build_prod Your tests passed on CircleCI!
Details
ci/circleci: checkout_code Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node4 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_node9 Your tests passed on CircleCI!
Details
ci/circleci: test_prod_standalone Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 80%)
Details
codecov/project 96.48% (+<.01%) compared to a938076
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@j-f1

This comment has been minimized.

Copy link
Member

commented Aug 3, 2018

Thank you for contributing to Prettier!

@ikatyang ikatyang added this to the 1.14.1 milestone Aug 7, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Nov 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.