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

[RFC] project options using package.json #1086

Closed
wants to merge 10 commits into from
Closed

[RFC] project options using package.json #1086

wants to merge 10 commits into from

Conversation

eXon
Copy link

@eXon eXon commented Mar 24, 2017

First, I have to say you are doing an awesome job on this project @vjeux and @jlongster !!!!

I believe this is related to #40 and #154

There is a big frustration when trying to use prettier: you don't know what are the project options. Even worse, if you are using the Atom plugin just like me, you're configuring it globally and honeslty, that sucks.

I know, you can format it your way and re-format it just before your commit and even automate this with some hooks. Honestly, it feels hackish. It just feels wrong.

Some people proposed to add a configuration file for prettier and it does seem like the core team don't want that. I can't blame them when looking at the root of my project, I have way too much files and adding one more would be silly. Especially considering prettier has no more than 7 options.

What I would like to propose is using the package.json file to be able to force options for your specific project. Prettier has only 7 options (love it that way) to override so it shouldn't be a big deal.

I made a proof of concept with the CLI using the working directory to search for the package.json. Within the editor plugins we could use the file directory to search for the package.json.

{
  "name": "my-package",
  // ...
  "prettier": {
    "singleQuote": true
  }
}

What do you guys think about that?

@vjeux
Copy link
Contributor

vjeux commented Mar 24, 2017

Look at #918 for the proposal we want to go forward to but haven't actually started working on it.

@eXon
Copy link
Author

eXon commented Mar 24, 2017

@vjeux I'm happy we are on the same page. I don't know how I missed that issue. I would love to help with this because I think this PR is more than just a POC. It's the actual feature minus some optimizations.

I went to your proposal line by line and he's my though:

  • Your proposal has the exact same format on package.json so that's awesome
  • It search for the package.json depth first just like you proposed
  • It stops at the first package.json with a prettier config (useful for people using things like lerna)
  • There is no caching on the package.json, just plain old fs.readFileSync (can be interesting to implement later on)
  • One major difference with your proposal is I don't intend to use the CLI for editors
  • The responsibility of merging the user options with the project options would not be within the core but rather within the CLI and editor itself
  • The core would contain a helper (getProjectOptions) to make sure we don't duplicate code
  • The CLI could use the working directory as a starting point to find package.json (we might or might not have one or multiple filenames here)
  • The editor would use the editing file as a starting point to find package.json
  • Because we don't use the CLI with everything, no headache on how to make those calls faster

I don't know inside out the project so I might overlook some problems.

@vjeux Am I on the right track?

src/util.js Outdated
@@ -305,9 +307,49 @@ function getPrecedence(op) {
return PRECEDENCE[op];
}

function getProjectOptions(searchDirectory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole thing can be replaced by https://www.npmjs.com/package/read-pkg-up except for the part to keep searching if no prettier-entry

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, or just https://www.npmjs.com/package/pkg-conf which is then even easier

@eXon eXon changed the title PROPOSAL: project options using package.json [RFC] project options using package.json Apr 1, 2017
@eXon
Copy link
Author

eXon commented Apr 1, 2017

@vjeux fixed a bug with non-undefined default options and using now pkg-config to get the package.json and dashify to convert the camelCase configs to dash so we can pass them to minimalist.

I've checked on NPM and they are widely used (over 300k and 500k download last month), still maintained and work very well.

@eXon
Copy link
Author

eXon commented Apr 29, 2017

Any update on this PR?

@vjeux
Copy link
Contributor

vjeux commented Apr 29, 2017

Hey I'm sorry, I need to have a conversation with @jlongster about the exact steps we need to do. We've been focused on the clear cut things so far.

@eXon
Copy link
Author

eXon commented Apr 29, 2017

No worry, I know how time-consuming OS is.

@zimme
Copy link
Member

zimme commented Jun 15, 2017

Any updates on this?

@vjeux
Copy link
Contributor

vjeux commented Jun 15, 2017

@zimme I haven't looked into it yet, but it's something that I do want to solve.

@simon04
Copy link
Contributor

simon04 commented Jun 19, 2017

In #98 (comment), @thasmo suggest using https://github.com/davidtheclark/cosmiconfig.

@azz
Copy link
Member

azz commented Jul 8, 2017

For those interested, I had a crack at implementing cosmiconfig in #2434.

@azz
Copy link
Member

azz commented Jul 9, 2017

Closing in favour of #2434. Thanks for getting the ball rolling!

@azz azz closed this Jul 9, 2017
@eXon eXon deleted the feature/projectDefault branch July 15, 2017 14:23
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants