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

service: fix handling of --option <value> #725

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

Conversation

alanpearce
Copy link

Running prettier --help shows options with their values separated with spaces.

Running prettierd with the options formatted as such doesn't work, but prettier doesn't differentiate between --opt=val and --opt val. Unfortunately my editor setup uses the space variety and isn't easy to change as it affects other formatting tools as well.

A better solution would be to somehow import prettier's option handling code, but I thought that might be a bit too much for this.

before:

bin/prettierd --tab-width 2 --stdin-filepath src/service.ts < src/service.ts # ❌
bin/prettierd --tab-width=2 --stdin-filepath src/service.ts < src/service.ts # ✅
bin/prettierd --stdin-filepath src/service.ts < src/service.ts # ✅
bin/prettierd src/service.ts < src/service.ts # ✅

after:

bin/prettierd --tab-width=2 --stdin-filepath src/service.ts < src/service.ts # ✅
bin/prettierd --tab-width 2 --stdin-filepath src/service.ts < src/service.ts # ✅
bin/prettierd --stdin-filepath src/service.ts < src/service.ts # ✅
bin/prettierd src/service.ts < src/service.ts # ✅

src/service.ts Outdated
Comment on lines 170 to 172
if (nextArg.done) {
throw new Error(`--${arg} expects a value`);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think the problem is that there are cases where this isn't true (some examples include --bracket-same-line and --single-quote), so it's hard to solve. We may need to actually use something that has the specific options and try to keep them in sync with prettier, which is a pain :(

Copy link
Author

Choose a reason for hiding this comment

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

I was playing around in the repl and got this out:

(await require("prettier").getSupportInfo()).options.filter(o => o.type === "boolean").map(o => o.name)
[
  'singleQuote',
  'bracketSpacing',
  'bracketSameLine',
  'singleAttributePerLine',
  'vueIndentScriptAndStyle',
  'semi',
  'experimentalTernaries',
  'jsxSingleQuote',
  'insertPragma',
  'requirePragma',
  'useTabs'
]

These are all the prettier options1 that don't take an argument (some have o.default: true, meaning the argument would have a no- prefix, like --no-semi).

This seems like something that can be worked with, but the arguments could be different depending on the resolved prettier version, which wouldn't be a problem, except that we don't know the version until we've parsed the arguments :^).

Given how prettierd doesn't support prettierd file.ts, I'm more inclined to suggest that --stdin-filepath <filename> should be required, which would be a breaking change, but far simpler.

Footnotes

  1. the prettier CLI has other arguments that don't take a value, but here we're calling the module

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm yeah I think that's an OK breaking change to make, as it simplifies things and makes prettierd command line interface compatible with prettier.

Do you want to give it a shot?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, will do!

@alanpearce
Copy link
Author

It's a shame iterators don't provide a peek() or prev() function; I had to use the ancient style of for loop :(

src/service.ts Outdated
Comment on lines 172 to 174
if (nextArg === undefined) {
throw new Error(`--${arg} expects a value`);
}
Copy link
Owner

Choose a reason for hiding this comment

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

do we still want this check?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. Made me realise I missed some edge cases.

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