panoply / vscode-liquid

💧Liquid language support for VS Code
https://marketplace.visualstudio.com/items?itemName=sissel.shopify-liquid
Other
173 stars 22 forks source link

Deprecate liquid.format.enable in favor of editor.formatOnSave and editor.defaultFormatter #132

Closed davidwarrington closed 11 months ago

davidwarrington commented 1 year ago

When trying to disable liquid formatting with this code VSCode reports that it doesn't recognise the setting:

{
  "[liquid]": {
    "liquid.format.enable": false,
  },
}

image

panoply commented 1 year ago

I can't recreate. Can you confirm no global workspace settings are defined?

panoply commented 1 year ago

What about simply toggling?

https://user-images.githubusercontent.com/7041324/207273050-5c2e9720-a9c3-4d1c-86f9-a815a2c7bf70.mov

davidwarrington commented 1 year ago

image

No workspace specific settings if that's what you mean?

I've tried toggling the option as per your video but it didn't work. It created the settings file in my workspace as expected but it didn't have any impact otherwise. I'll share a video afterwards to show you, I'm about to head into a meeting now though

panoply commented 1 year ago

Please do. Thanks.

davidwarrington commented 1 year ago

So I've been struggling to replicate this consistently since coming back to it. I've been trying to record a barebones example and had mixed results. Hopefully this should demonstrate the issue I've been having though. The fact I've not been able to replicate it consistently is just more concerning

https://user-images.githubusercontent.com/9138568/207323074-15fe453c-49f6-470c-908c-414b1574be79.mov

panoply commented 1 year ago

Interesting. I am guessing there is a conflict happening or maybe it is the behaviour of setting configuration within user workspace settings and then in the projects .vscode/settings.json configuration.

Can you check your user workspace settings and see if you can locate any language options, specifically, one which is setting the liquid default formatter, eg:

{
   "[liquid]": {
      "editor.defaultFormatter": "sissel.shopify-liquid"
   }
}

In any sense, this logic needs some re-thinking. As always, curious your thoughts here. The liquid.format.ignore should maybe be treated as a kill-switch and if present no matter if user workspace of project level workspace (ie: .vscode/settings.json) that formatting should never be applied.

davidwarrington commented 1 year ago

I think I've figured this out because of a yml file...

Recently I'd enabled editor.formatOnSave whilst working on a Node app and had forgotten to disable it (this is why I really should do project specific settings more often). Since setting that to false "liquid.format.enable": false is now consistently being respected for me

panoply commented 1 year ago

I think you might be onto something here though as @MaxDesignFR recently reported a similar issue in the discord. I'll have a deeper look to see if is on the extension end or something else.

MaxDesignFR commented 1 year ago

I also have"editor.formatOnSave": true, my understanding is that "liquid.format.enable": false should not format the liquid document, whether editor.formatOnSave is set to true or false.

panoply commented 1 year ago

@MaxDesignFR thanks for chiming in 🙏🏼 @davidwarrington What are your thoughts here? should liquid.format.enable act as the kill switch and override editor.formatOnSave or alternatively is liquid.format.enable complicating this and maybe it should be deprecated? Curious on your thoughts as you tend to always be really concise on these topics.

davidwarrington commented 1 year ago

I think it would make sense to deprecate it in favour of editor.formatOnSave and

{
  "[liquid]": {
    "editor.defaultFormatter": "sissel.vscode-liquid"
  }
}

Reason being is that's how prettier and probably other formatter for VSCode are set up.

Also since Shopify how now released the 1.0.0 of their prettier plugin, having your extension fighting with it might hurt adoption.

I know for example my team are most likely to stick with the official Prettier plugin because its Shopify backed, and we have some team member who use other editors too

panoply commented 1 year ago

Good point.

The Prettier Plugin solution will always be favoured and moving developers to more complex solution with more refined control is going to be an uphill battle (no doubt). I guess this starts with adopting existing and known handlings which pertain to text editor automated beautification.

It's interesting how the opinionated conventions have been so readily welcomed, especially when dealing with a template language like Liquid. The Prettier plugin is still lacking when dealing with structures (eg: <div {{ attr }}={{ value }}></div> or <div data-{% if %}{{ x }}="foo"{% else %}{{ bar }}{% endif %}></div>) is not possible. I like to believe there will be a shift in consensus over time and more so when Prettify is more stable, but then again maybe teams prefer the elementary.

I'll deprecate this option in next minor v3.3.0 and instead move to options defined with editor.formatOnSave and language specific workspace configurations (ie: editor.defaultFormatter).

panoply commented 11 months ago

Closing as most folks should've adopted this by now. Thanks again brother.