Allow plugins to override default options by czosel · Pull Request #3991 · prettier/prettier

czosel · 2018-02-17T15:07:23Z

This is a first attempt to handle #3972. See prettier/plugin-php#52 for a usage example. I attached defaultOptions to the printer to reuse existing logic (getPrinter). Like that, only files handled by the respective plugin should be affected by the changed defaults.

This doesn't handle multiparser support, though - but maybe that's something to be handled in the future when there is a concrete use case?

azz · 2018-02-18T01:02:59Z

I think we want the API to be

exports.defaultOptions = {...}

Rather than:

exports.printers.php.defaultOptions = {...}

Because we may want to show this information in the CLI. e.g.:

$ prettier --help tab-width
--tab-width <int>

  Number of spaces per indentation level.

Default: 2
Plugin defaults:
* @prettier/plugin-php: 4

Do you have any thoughts on this @ikatyang?

czosel · 2018-02-19T09:15:42Z

That makes sense! I just pushed some changes that add the plugin's options to a new property pluginOptions in supportOptions, which should make it easy to display them in the CLI's help. I read from the same object in options.js to extract the current defaults.

In this implementation a plugin's default options are registered under the name of it's first printer - which works because currently all plugins only have one printer. Maybe there's a better solution, for example a name property on the plugin?

If you think this is generally the right way to move forward, i'll continue by adding some tests!

ikatyang · 2018-02-22T16:24:31Z

+      );
+      const pluginDefaults = filteredPlugins.reduce((reduced, plugin) => {
+        const printerName = Object.keys(plugin.printers)[0];
+        reduced[printerName] = plugin.defaultOptions[option.name];


We probably need to attach the plugin name (package name, plugin filepath, or the name property if provided) in load-plugins.js and use it here as an ID.

prettier/src/common/load-plugins.js

Line 6 in 9643d33

function loadPlugins(plugins) {

ikatyang · 2018-02-22T16:24:52Z

+  const pluginDefaults = supportOptions
+    .filter(
+      optionInfo =>
+        optionInfo.pluginDefaults && optionInfo.pluginDefaults[options.parser]


Same here, since the parser name might not equal to the printer name.

czosel · 2018-02-22T20:06:45Z

@ikatyang Right - i updated the implementation.

ikatyang · 2018-02-23T02:47:46Z

      const pluginPath = resolve.sync(plugin, { basedir: process.cwd() });
-      return eval("require")(pluginPath);
+      const resolved = eval("require")(pluginPath);
+      resolved.name = plugin;


We should make a clone for it so as to avoid unexpected side effects.

return Object.assign({ name: plugin }, resolved);

ikatyang · 2018-02-23T02:53:30Z

-  return `${header}${description}${choices}${defaults}`;
+  const pluginDefaults =
+    option.pluginDefaults && Object.keys(option.pluginDefaults).length
+      ? `\nPlugin defaults:${Object.keys(option.pluginDefaults).map(


~~Could you add tests for it?~~ (added in 763eaa4)

ikatyang · 2018-02-23T02:56:18Z

+      {}
+    );
+
+  Object.assign(defaults, pluginDefaults);


Let's not mutate this object, assigning to a new object would be better.

ikatyang · 2018-02-23T02:59:38Z

+        reduced[plugin.name] = plugin.defaultOptions[option.name];
+        return reduced;
+      }, {});
+      return Object.assign(option, { pluginDefaults });


~~Could you add tests for it?~~ (added in eddedde)

ikatyang · 2018-02-23T03:22:45Z

+  const pluginDefaults =
+    option.pluginDefaults && Object.keys(option.pluginDefaults).length
+      ? `\nPlugin defaults:${Object.keys(option.pluginDefaults).map(
+          key => `\n* ${key}: ${option.pluginDefaults[key]}`


We should call createDefaultValueDisplay(value) here, though there is no non-primitive value in core options.

ikatyang · 2018-02-23T13:43:06Z

+      {}
+    );
+
+  Object.assign({}, defaults, pluginDefaults);


It seems you forgot to get the assigned defaults and use it below 😂, and also we need some tests for it.

Oops, thats embarassing 😅 Probably still have to figure out how to rewrite it that i don't have to use let (and test is also still missing)

You can just assign it to a new variable, e.g. mixedDefaults or something else:

const mixedDefaults = Object.assign({}, defaults, pluginDefaults); // Object.keys(mixedDefaults) ...

right - done.

ikatyang · 2018-02-23T13:48:40Z

+    "--parser=bar"
+  ]).test({
+    status: 0,
+    stdout: `--tab-width <int>


It'd be better to just snapshot it (by removing the stdout key) since it's not a simple string and may contains style changes in the future.

Of course - didn't think that doing that would be so easy 👍

azz · 2018-02-24T06:47:33Z

+
+Default: 2
+Plugin defaults:
+* ../plugins/automatic/node_modules/prettier-plugin-bar: 4


Could we use the package.json name here? (Happy to do this in a follow up PR though)

That's the result from --plugin=../plugins/automatic/node_modules/prettier-plugin-bar, I guess it should be fine?

--plugin=path/to/plugin.js (path)

Plugin defaults: * path/to/plugin.js: 4

--plugin=prettier-plugin-bar (package name)

Plugin defaults: * prettier-plugin-bar: 4

Ah, cool. I'm happy with that. Does it work for plugins that are automatically resolved?

It should work. @czosel could you add a test for that? just clone the test case and set its cwd to path/to/plugins/automatic then remove the --plugin=xxx part.

azz · 2018-02-24T10:57:19Z

      print: path => concat(["bar+", path.getValue().text])
    }
+  },
+  defaultOptions: {


@ikatyang mentioned this above, but could you add an integration test to ensure that this tabWidth is applied?

azz · 2018-02-24T10:57:58Z

LGTM other than missing an integration test.

czosel · 2018-02-24T11:07:11Z

    }
  });

+  Object.assign(rawOptions, pluginDefaults);


Just realized that rawOptions already contains the default options, so assigning the "mixedDefaults" only if the option key is not present in rawOptions (line 73) won't work. I added some tests for it.
Edit: Like this, overriding won't work anymore - i'll try to figure out the right place to assing the plugin options.

Hmm, if I'm not mistaken the order of option application should be

core defaults < plugin defaults < cli / file overrides

That would mean that we already have to find out which plugin is used before CLI options are applied, so the logic has to be moved to somewhere around here:

https://github.com/prettier/prettier/blob/master/src/cli/util.js#L224

What doesn't make this easier is the fact that the active plugin depends on the astFormat option. Maybe we should call minimist twice:

1) minimist(cli options, coredefaults) 2) determine active plugin 3) minimist(cli options, mixeddefaults)

czosel · 2018-02-25T13:49:22Z

+        return reduced;
+      }
+      return Object.assign(reduced, { [optionInfo.name]: optionInfo.default });
+    }, Object.assign({}, optionsModule.hiddenDefaults));


I managed to get the tests passing by adding this workaround - but I'm not really happy with it. I could imagine that there's a case where this breaks, and even if there isn't it doesn't look very clean to me.

I'd be glad if someone could explain the bigger picture on the way defaults are handled right now: To me, the two-step process of applying defaults seems confusing:

CLI defaults are applied in cli/util.js

prettier/src/cli/util.js

Line 226 in 9ee113c

minimist(

Defaults are applied if an option is null in main/options.js

prettier/src/main/options.js

Line 58 in 9ee113c

Object.keys(defaults).forEach(k => {

I saw that there are some use cases where the core options are still null in step two, but I'm not sure what those use cases are, and consequently how the proper process of applying defaults should look like in the future.

Thanks 😄

czosel · 2018-02-26T06:54:49Z

Workaround is reverted, tests are passing 🎉

ikatyang

LGTM, but I'd like someone else to take another look.

czosel · 2018-02-27T12:22:14Z

Awesome, can't wait! 😉
I also just added some basic docs for options and defaultOptions.

azz

LGTM!

j-f1 · 2018-02-27T14:06:42Z

+    description: "Move open brace for code blocks onto new line."
+  }
+}
+```


Can you document each key in the option?

Of course! Can you help my out with the category option? I'll do this in a new PR.

j-f1 · 2018-02-27T14:06:51Z

+
+If your plugin requires different default values for some of prettier's core options, you can specify them in `defaultOptions`:
+
+```


czosel force-pushed the plugin-default-options branch 2 times, most recently from 6c9077e to f0562a0 Compare February 19, 2018 12:36

ikatyang reviewed Feb 22, 2018

View reviewed changes

ikatyang reviewed Feb 23, 2018

View reviewed changes

azz reviewed Feb 24, 2018

View reviewed changes

czosel commented Feb 24, 2018

View reviewed changes

azz reviewed Feb 24, 2018

View reviewed changes

Comment thread tests_integration/plugins/defaultOptions/plugin.js Outdated

czosel force-pushed the plugin-default-options branch 3 times, most recently from 7305253 to 7a20fcf Compare February 25, 2018 12:26

czosel commented Feb 25, 2018

View reviewed changes

refactor(cli): defer default value applying

7cc15e3

ikatyang mentioned this pull request Feb 26, 2018

refactor(cli): defer default value applying #4045

Closed

czosel and others added 11 commits February 26, 2018 07:29

Allow plugins to override default options

943d68b

Move "defaultOptions" to top level of plugin

3464f6a

Simplify implementation

59a392e

Attach plugin name

3f04679

Add pluginOptions to cli help

14413e2

Update snapshots

95889bd

Code review (immutable style)

b6d1296

Add test for help output

004d70a

Use snapshot test, fix Object.assign

a31cac0

Refactor to immutable style

6fc4cb7

Add test case for automatic plugin resolution

ed9bd1e

czosel and others added 4 commits February 26, 2018 07:29

Add tests for applying and overriding default opts

eddedde

Remove "since" option

4689811

Only set defaults for CLI args when no pluginDefaults are present

fdb6905

Revert workaround, rebase to prettier#4045

7adffba

czosel force-pushed the plugin-default-options branch from 1a2ef07 to 7adffba Compare February 26, 2018 06:32

ikatyang approved these changes Feb 27, 2018

View reviewed changes

Add basic documentation for options and defaultOptions

3facd68

azz approved these changes Feb 27, 2018

View reviewed changes

azz merged commit d05a29d into prettier:master Feb 27, 2018

czosel deleted the plugin-default-options branch February 27, 2018 13:47

j-f1 reviewed Feb 27, 2018

View reviewed changes

ikatyang mentioned this pull request Feb 28, 2018

Change option defaults in plugins #3972

Closed

dependabot-preview Bot mentioned this pull request Mar 17, 2018

Bump prettier from 1.11.0 to 1.11.1 injms/inj.ms#7

Closed

renovate Bot mentioned this pull request Mar 22, 2018

Update prettier to 1.12.1 sbardian/DebtTrackerAPI#28

Merged

lock Bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 18, 2019

lock Bot locked as resolved and limited conversation to collaborators Jan 18, 2019


		If your plugin requires different default values for some of prettier's core options, you can specify them in `defaultOptions`:

		```

Uh oh!

Conversation

czosel commented Feb 17, 2018

Uh oh!

azz commented Feb 18, 2018

Uh oh!

czosel commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czosel commented Feb 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikatyang Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikatyang Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azz commented Feb 24, 2018

Uh oh!

czosel Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czosel Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

czosel Feb 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czosel commented Feb 26, 2018

Uh oh!

ikatyang left a comment

Choose a reason for hiding this comment

Uh oh!

czosel commented Feb 27, 2018

Uh oh!

azz left a comment

Choose a reason for hiding this comment

Uh oh!

czosel commented Feb 19, 2018 •

edited

Loading

ikatyang Feb 23, 2018 •

edited

Loading

ikatyang Feb 23, 2018 •

edited

Loading

czosel Feb 24, 2018 •

edited

Loading

czosel Feb 24, 2018 •

edited

Loading

czosel Feb 25, 2018 •

edited

Loading