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

Refactor options #13746

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Refactor options #13746

wants to merge 13 commits into from

Conversation

fisker
Copy link
Member

@fisker fisker commented Oct 28, 2022

Description

After taking a close look at the options, I found it's messy.

Every language has own options, I guess the initial idea is to only pass those options when doing print. But it not actually working that way, for example trailingComma is defined in JavaScript, but CSS printer response to it too. Even if we want it strictly only pass supported options to plugin, it should be defined in parser/printer, not plugins, because plugins can have multiple printers.

I decided to move all those options to formatOptions (we don't have any "parseOption") for now.

I also found that, currently options in plugins can even override any other options, except "coreOptions". So, I think it's possible to define a null plugin, only contains {singleQuote: {default: true}} will make us use singleQuote by default. But this is not easy to fix, because we still need support plugins to add new options.


Update:
Tested.

// foo.js
export default {
  options: {
    singleQuote: {default: true, type: "boolean"}
  }
}
// .prettierrc
plugins:
  - ./foo.js
>node bin/prettier index.js
export * from './src/index.js';

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@fisker fisker marked this pull request as ready for review October 28, 2022 09:09
# Conflicts:
#	src/common/common-options.js
#	src/index.js
#	src/language-css/options.js
#	src/language-graphql/options.js
#	src/language-html/options.js
#	src/language-js/options.js
#	src/language-markdown/options.js
#	src/language-yaml/options.js
#	src/main/support.js
#	tests/integration/__tests__/bundle.js
@thorn0
Copy link
Member

thorn0 commented Nov 6, 2022

All the options are gone on the playground.

I guess the initial idea is to only pass those options when doing print

Probably not. Maybe the idea was simply to categorize them.

it should be defined in parser/printer, not plugins, because plugins can have multiple printers

How would configuration files and CLI work in that case? Would users need to specify which parser/printer each option is for?

think it's possible to define a null plugin, only contains {singleQuote: {default: true}} will make us use singleQuote by default

Does that really need fixing? The biggest problem with the current state of things I can see is that multiple plugins may define options with the same name but different set of values. It's a typical problem with any global name space, but sometimes avoiding such problems creates quite a lot of complexity (e.g., modules in JS). What I'm saying is that sometimes documenting a known issue and, for example, a recommendation to use prefixes for plugin options is a good enough solution.

@fisker fisker changed the base branch from next to main April 23, 2023 01:19
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.

None yet

2 participants