Prettier-atom: Request: remove merging of explicit configurations with atom-prettier settings

[olsonpm]

I just spent some time looking into why my prettier settings in package.json weren't being respected, and it's due to them being assigned over atom-prettier settings. I'm wondering what the use-case is for

1. allowing users to only format if a prettier config is found and
2. assigning the prettier config over those in atom-prettier.

i.e. why would I want to require an explicit configuration while also using the one local to my editor? I assume the use-case for allowing users to require both an explicit prettier instance and settings would be to ensure consistent formatting should they move to a different editor or machine.

Even if I set atom-prettier's settings to the defaults, this could become problematic should the defaults in the local prettier instance be different.

Another less breaking solution would be to add an option to ignore atom-prettier's configuration settings.

I'm willing to create a PR if either solution is agreeable.

[robwise]

my prettier settings in package.json weren't being respected

@olsonpm I'm a bit confused. The way it's supposed to work is that the settings in your package.json _should_ be respected? We merge them in _after_ the Atom settings, so they should take precedence.

Answers to those questions:

1. Using prettier in a repo where it's not been adopted by the entire team will cause massive diffs on any file you work on, so you can use this option as a way of telling Atom when to use prettier and when not to.
2. For pretty much the same reason as #1 you would never want to have two developers using prettier on the same project with different settings. You'd end up with massive diffs as each developer touched a file after the other one. You'd have two different code styles depending on who touched each file last. So, if the project has an explicitly defined prettier config, then we assume those are the settings that the project is using. If there are certain options inside of the config that are not defined, then that indicates it's up to the individual which style they prefer and we fall back to the settings in Atom, but I would not recommend this because of the diff problem.

Maybe what you're suggesting/questioning is similar to point raised by #218?

[olsonpm]

Thanks for the reply.

Let me think about what you wrote, but regarding:

The way it's supposed to work is that the settings in your package.json should be respected? We merge them in after the Atom settings, so they should take precedence

I was expecting them to not be merged with the atom-settings configurations. By merging them with atom-settings I have no way to fall back to prettier defaults.

[olsonpm]

Using prettier in a repo where it's not been adopted by the entire team will cause massive diffs on any file you work on, so you can use this option as a way of telling Atom when to use prettier and when not to.

By merging project settings with those local to my editor prevents project compatible settings though, correct?

For # 2 - we agree this is not ideal.

Thanks for the link to the other issue. Seems I'm suggesting exactly what you wrote there

a change that would work regardless of whether the user is using prettier-eslint or not. Basically, if we detect that prettier is in the package.json and this option is enabled, we just don't pass any options from prettier-atom.

[SavePointSam]

I can also agree that if a configuration is found in the project; then we should ignore using the settings in prettier-atom altogether and let Prettier fill in the gaps. This is how the system is described to work in the Prettier docs and we should respect that as we are just a connector.

I normally put all the configuration settings in, even if they are just the defaults, so I’ve never seen this. However, I would be extremely frustrated by the situation described here.

It should work like:

• if any Prettier configuration is found
  
  
  
  • ignore prettier-atom settings and let Prettier sort through any missing configuration
  
  
• if no configuration is located
  
  
  
  • and ‘only format if Prettier is in project’ is true
  
  
  • only format using a found configuration
  
  
  • and ‘only format is Prettier is is project’ is false
  
  
  • format using the rules in prettier-atom settings
  
  

We should probably also consider allowing the user to completely disable package settings as well. I’ve been wanting a way to make sure I just use Prettier’s defaults no matter what I put in the prettier-atom settings.

[olsonpm]

That logic sounds good to me. I'll get a PR implementing that so we have something concrete to discuss.

In the PR I'll also modify the debug command to be explicit about what configurations would be applied to a given file.

[robwise]

I think one of the issues we originally had when prettier implemented its configuration files was that it considered an empty configuration file as having no rules defined. In this case, we weren't able to tell the difference between a default configuration and no configuration at all. That's sort of the arcane history explaining why it got implemented the way it did. Hopefully, they've changed this (I think they might have fixed it when I brought up the issue during an editorconfig integration on Prettier, but I can't really remember).

[SavePointSam]

AFAIK and what I would assume Prettier does is take in any settings and for those it doesn’t find it fills in the defaults. In a sort of Prettier config > .editorconfig > Prettier defaults manner.

[olsonpm]

I've been looking into the implementation and catching up on some issue reading - Prettier uses cosmiconfig which enforces a single source of configuration (thank god). The prettier config now allows for a property ecitorconfig which allows us to remove the manual buildEditorConfig* files. Unfortunately editorconfig's parse may parse many configuration sources and doesn't currently expose which. This creates a headache for #178.

Regardless, the implementation I'm starting with is:

--  return {
--    ...optionsFromSettings,
--    ...buildEditorConfigOptionsIfAppropriate(editor),
--    ...getPrettierConfigOptions(editor),
--  };
++ return getPrettierConfigOptions(editor) || optionsFromSettings;

btw @SavePointSam, when you mentioned

if no configuration is located
  - and ‘only format if Prettier is in project’ is true
    - only format using a found configuration

You meant to format using the project prettier version's defaults as the found configuration would be null/empty - correct?

Edit:

The prettier config now allows for a property ecitorconfig which allows us to remove the manual buildEditorConfig* files.

This is incorrect, I assumed it would be a project setting but because of what robwise pointed out (empty === nonexistent) I created a cosmiconfig issue to address that. This is blocked until the cosmiconfig issue is addressed.

[SavePointSam]

@olsonpm, I meant that you only format if a Prettier configuration was found in the current project path. It can still be a subset configuration that would need to be cast against Prettier defaults. However, if no configuration is found, do not format at all.

Also note, I’ve written a new API for Prettier called resolveConfigWithFilePath. It may be worthwhile to look into that, though I’m not 100% in hownit would fit into your ideas.

[robwise]

@olsonpm I think at least being able to see _what_ options are used, even if we can't see _why_ because of the editorconfig ambiguity, would be a huge win for us.

[olsonpm]

Sounds good about exposing what options are used. And I was planning on using cosmiconfig's load to get the file until your resolveConfigWithFilePath PR is merged.

Unfortunately I'm realizing the combinations of configurations this tool allows can lead to some pretty unexpected behavior in a team setting. e.g. I don't see any applicable scenario where someone should use a global installation of prettier with a local configuration, or a local installation with prettier-atom settings. I'll ignore this for now though - it's just difficult wrapping my head around the use-cases this tool is meant to address.

[robwise]

@olsonpm I agree that both of those use cases wouldn't make sense to me personally.