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

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

Closed
davidwarrington opened this issue Dec 13, 2022 · 13 comments
Labels
📢 Announcement Issue is an announcement or notice ⚡Enhancement New feature or request

Comments

@davidwarrington
Copy link
Collaborator

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
Copy link
Owner

panoply commented Dec 13, 2022

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

@panoply panoply added Bug Something isn't working in the vscode extension ⚠️ Monitoring labels Dec 13, 2022
@panoply
Copy link
Owner

panoply commented Dec 13, 2022

What about simply toggling?

Screen.Recording.2022-12-13.at.09.59.32.mov

@davidwarrington
Copy link
Collaborator Author

davidwarrington commented Dec 13, 2022

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
Copy link
Owner

panoply commented Dec 13, 2022

Please do. Thanks.

@davidwarrington
Copy link
Collaborator Author

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

vscode-liquid-issue.mov

@panoply
Copy link
Owner

panoply commented Dec 13, 2022

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
Copy link
Collaborator Author

davidwarrington commented Dec 13, 2022

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
Copy link
Owner

panoply commented Dec 13, 2022

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
Copy link
Contributor

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
Copy link
Owner

panoply commented Dec 13, 2022

@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.

@panoply panoply removed the Bug Something isn't working in the vscode extension label Dec 13, 2022
@davidwarrington
Copy link
Collaborator Author

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
Copy link
Owner

panoply commented Dec 13, 2022

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 panoply changed the title Can't disable Liquid formatting on a per-language level Deprecate liquid.format.enable in favor of editor.formatOnSave and editor.defaultFormatter Dec 13, 2022
@panoply panoply added ⚡Enhancement New feature or request 📢 Announcement Issue is an announcement or notice and removed ⚠️ Monitoring labels Dec 13, 2022
This was referenced Dec 18, 2022
panoply added a commit that referenced this issue Dec 26, 2022
@panoply
Copy link
Owner

panoply commented Nov 7, 2023

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

@panoply panoply closed this as completed Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📢 Announcement Issue is an announcement or notice ⚡Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants