vscode-liquid icon indicating copy to clipboard operation
vscode-liquid copied to clipboard

3.0 update syntax highlighting not as good as before

Open muchisx opened this issue 1 year ago • 23 comments

I think its overwritting the syntax highlighting from the Shopify Liquid Extension, and this extension is not as good as the one from Shopify when it comes to syntax highlighting, so maybe give it less priority (I dont really know how the priority works).

Before it was able to detect what a comment inside a liquid block is and different segmented objects separated by dots ( . )... + a whole lot of stuff.

Now is just a regex mess, can we copy the syntax highlighting from Shopify Liquid or atleast give this extension less priority when having both installed?

muchisx avatar Sep 29 '22 14:09 muchisx

Same here....

haroldao avatar Sep 29 '22 14:09 haroldao

@muchisx https://github.com/panoply/vscode-liquid#continue-using-v230 Let's downgrade 😇

haroldao avatar Sep 29 '22 14:09 haroldao

image I downgraded to 2.3.0 and everything is back now 🎉

haroldao avatar Sep 29 '22 14:09 haroldao

@haroldao Oh I was wondering where the [schema: ----] snippents went, I thought they were from another extensions, so they also dissapeared with this update? damn. I actually like some of the 3.0 features but I guess we'll have to downgrade for now while this get fixed ?

muchisx avatar Sep 29 '22 14:09 muchisx

Yeah, so this version is targeting Liquid grammars opposed to injecting within HTML grammars. In v2.3 the only reason grammars were injected was for HTML intelliSense but as of v3 HTML intellisense is made available in Liquid files. The Shopify extension also target Liquid grammars so this is why things will be conflicting.

It is easy to fix and though I'd be happy to forward gramars to Shopify's variant, I prefer to handle everything in an isolated manner as the grammars need to extend support to non Shopify projects.

Why this is better?

Injecting into HTML was problematic and caused troubles for projects that were not Liquid. The benefit of the upgrade allows for users to get code completions in .liquid files such as HTML tags, JavaScript and CSS completions in <script> and <style> tags plus a whole lot more.

Fixing the issues

I can align the grammars to highlight whatever you need, send me a screenshot of what is conflicting and what you need or prefer and I will fix it today

Schema Snippets

@muchisx Ouff, it seems I forgot to add them into the release, I will fix this in next patch. Keep in mind I will deprecate them in one of the next minors because the supersede capability of Liquify will be made available and you will get intellisense embedded regions, eg:

https://youtu.be/KfUnYiuihw8

panoply avatar Sep 29 '22 15:09 panoply

@muchisx @haroldao

It would be huge help if you guys could shoot me a list of syntax highlights that you prefer or differences you have come across so I can align them. Also note that the grammars in this release are highly customisable and you can extend them to your preferences using the vscode editor.tokenColorCustomizations option.


If you are keen to try a different theme, you can have a look at Potion which I maintain and is highly configured to Liquid.

panoply avatar Sep 29 '22 15:09 panoply

@panoply Awesome explanation, thank you and I understand!

Alright strap on (I will edit this comment with more syntax since I'm collecting one by one):

Keep in mind I edited the colors of some stuff in my settings.json but the scopes from your grammars remains the same, just the colors I changed a bit.

Also worth noting, I'm disabling Shopify Liquid extension for this so no syntax interference should happen.

Liquid Syntax Improvements

1. Comment inside liquid tag

  • 1 and 2 Should be start and end of comment and everything inside should be commented out
  • 3 should be ignored since is inside a comment block
  • 4 and 5 should also be ignored since they are inside a comment block

1 comments inside liquid tag

2. Entity names inside liquid tag

  • 1,2 and 3 should be scoped as entity names ( I think, if you have a better proposal you know more than me)
  • 4 and 5 same as above but just separating because weirdly they are being scoped differently

2 Entity names inside liquid tag

3. Render tag inside liquid and outside

  • 1 Should be scoped as 2.
  • 3 Should have its own scope, right now is falling under 'punctuation.output'.
  • 5 Should be scoped by 4, this is happening because the end of the render tag does not contain a comma ( , ) so it doesnt perceive 5 as punctuation.output but preferebly 4 would even have its own scope because from the name punctuation output I think it belongs to only things like section.settings.something, when you're navigation objects, right ?

3  Render tag inside liquid and outside

4. Logical properties and operators

  • 1 (true and false) should be scoped differently, i see that inside the if tag you have them scoped as constant.language.liquid, but after the equals ( = ) in the assign variable they are scoped differently.
  • 2,3,4 and 5 are being scoped as variable.parameter, in my perspective they should be colored just as the other operators ( =, ==, !=, >..etc) which is keyword.operator.{OPERATOR}.liquid.

4  Logical Properties and operators

5. 'as' being scoped differently when start of word

  • 1 and 4 when they are the start of word or when is a single word ('as') being scoped as variable.parameter
  • 2 and 5 whole word is cut and does not include the start of the word 'as', only what follows
  • 3 'as' works as expected when is not the start of the word This not only happens in the assign tag, showing the render too to demonstrate that is more a global tag issue I think.

5  'as' being scoped differently when start of word

Shopify-only issues/suggestions

If I understood correctly, the extension is heading towards Liquid in general and not Liquid+Shopify only, so I think these changes would only apply to Liquid+Shopify users like me !

A. Known Shopify objects highlight

Here's v2.3 vs v3.0 comparaison of how they looked (none of them perfect but v2.3 closer to be)

So, known objects like 'settings, collections, product, etc...' are scoped differently when navigating through objects in the v2.3, in the v3.0 only the first object will be highlighted and when is being accessed like and array it doesnt.

Note that 2.3 isnt perfect either because doesn't recognize 'section' as a known object but it actually is.

Also, when referenced elsewhere, like for example as argument in the render tag render 'x', product: product - the second product would be styled as a known shopify object which was cool ! Note that 2.3 wasn't perfect here because the parameter name is also being scoped as known shopify object when it should be just scoped as a parameter, only the argument should be scoped as a known shopify object.

A  Known Shopify objects highlight

B. Better [] {} () inside schema

I'm not entirely sure if the schema is a Shopify-only thing, I can move it up if it isn't, but yeah, somehow, and I'm also not sure if it has to do anything with the syntax highlighting but here it goes:

When using the Shopify Liquid extension only it allows vscode to use its feature of color coding nested {} [] which I find really usefull for orientation inside the schema!

Note that the start and closing { } are with both Shopify Liquid and Liquid extension are being a scoped as punctuation.definition.dictionary.{begin or end}.json

B  Better Bracers inside the schema

muchisx avatar Sep 29 '22 15:09 muchisx

Ah! I see. The liquid tag is something I didn't scope, I will get onto this now. Sit tight and keep the updates coming! Thanks so much for the time you are taking to help. I really appreciate it.

panoply avatar Sep 29 '22 16:09 panoply

@panoply added a couple more stuff, I'll keep editing more through the day ! thank you for your time too 🙏🏼

muchisx avatar Sep 29 '22 17:09 muchisx

@muchisx Great stuff here! I've got the comments completed and will work through the others. I will have you as co-author on this and reference commits on issue.

panoply avatar Sep 29 '22 17:09 panoply

@muchisx Do you have any other Liquid extensions enabled? The current grammars are not scoped in such a way in some cases. Are you running this extension isolated?

panoply avatar Sep 29 '22 19:09 panoply

@panoply Finished! I think I covered everything I noticed so far, If I find something else I'll edit and let you know.

I disabled Shopify Liquid for all these comments so nothing else should be interfering, let me test with only Liquid extension enabled, which cases should I compare again?

muchisx avatar Sep 29 '22 19:09 muchisx

@panoply I tested again with all extensions disabled and it scoping the same, did I miss something?

muchisx avatar Sep 29 '22 19:09 muchisx

Ouff. Ignore me, I was testing against the refined grammars 🫠


I will push these fixes tonight plus a couple of additional that improve the syntax support overall. I notice now that a lot of the existing logic implemented was for other Liquid variations, ie: Jekyll/11ty but it seems at second glance I can still support these without sacrificing other variation support.

I am little hesitate on bringing back chained highlights (Known Shopify objects highlight) but I will see what I can do. It might need to wait until future releases as the employed logic in Liquify applies this feature semantically.

Better [] {} () inside schema

Regarding this logic, it actually is as vscode feature editor.bracketPairColorization which is disabled by default in Liquid, you can enable but it does effect injections applied in external languages, which is why I disabled it.


Overall this was a brilliant issue and has been a huge help for this extension. I will have you as co-author. If you like, maybe join a few of us in the Liquify Discord.

panoply avatar Sep 29 '22 19:09 panoply

@panoply Awesome! Will surely join the discord! Thank you for inviting and taking the time to do this !

As for the "Known Shopify objects highlight" I think you're right, maybe get opinions from other people since its a subjective suggestion, I might like it, others not !

Looking forward to get the update 🙏🏼

muchisx avatar Sep 29 '22 19:09 muchisx

@muchisx oke, so working through the objects. A lot can be done here. Given the project is using the Liquid Specification created for Liquify means that there is known context for every single object available via the Shopify Liquid variation.

This means that objects which return different outputs can be reflective in the grammers. For example, let's take the {{ current_page }} object which only only ever returns a number value. The grammars could reflect a number scope for this object, eg:

Screenshot 2022-09-29 at 22 25 53

This can also be applied for different object types take for example {{ page_title }} which is a string and only ever outputs string types, eg:

Screenshot 2022-09-29 at 22 30 16

I would vote for this logic to be applied for those objects, as most always contain properties it would only highlight the few which do not. For those which do not I think this would be a nice addition as it will inform when it is being called or used what type it is.

Note Whenever a value is re-assigned the highlighting will fallback to punctuation, eg:

Screenshot 2022-09-29 at 22 34 04

panoply avatar Sep 29 '22 20:09 panoply

@panoply I like this approach, how would it style objects that return other objects? Like the products that returns a shopify productsDrop object if I remember correctly?

muchisx avatar Sep 29 '22 20:09 muchisx

I'm not sure I've heard of drops, do you mean something like:

{{ object1.object2.object3 }}

or ones which return something like an array which can either be iterated or referenced, eg:

{{ all_products['some-title'] }}

{% # or alternatively you can iterate %}

{% for product in all_products %}

{% endfor %}

panoply avatar Sep 29 '22 21:09 panoply

@panoply Yes like an array, I guess is just like Shopify calls it, the products object contains multiple product and Shopify calls it productsDrop or something like that but its definetly an array you can iterate through

you can do like

for product in collection.products
...
endfor

muchisx avatar Sep 29 '22 21:09 muchisx

Ah! Yes, this is something I would need to determine to the best scope of grammar to use. Because we are not dealing with a real programming language in Liquid, I don't believe things need to be very strict in this area. Going back to the Liquid Specifications, things could get very creative here.

Let's take deeply nested objects, here is this example I am using a mixtures of variable.language.object.liquid, support.type.object.liquid and string.other.liquid as scopes to infer the values. This way when you are referencing an object, you will know when you hit (for example) a string type. Depending on what makes most sense.

https://user-images.githubusercontent.com/7041324/193145493-471f2122-c24b-451a-91d2-0819a464187a.mov

This can be done a lot of different ways, here it is reversed.

Screenshot 2022-09-29 at 23 36 36

You could also implement this on a level basis, so (for example) only apply coloured type scope on the properties on a depth of 1 from the last value, eg:

Screenshot 2022-09-29 at 23 39 11

Where the user value is inferred to a meta scope, removing its highlight but the image because it contains 1 more value from the end (src) the highlight is applied.

panoply avatar Sep 29 '22 21:09 panoply

Actually, let's not get too creative. I think it is best to follow similar logic as was in 2.3 but do things a little more elegant on the ending values and infer their types accordingly. If a Products Drop (array) is referenced either use variable.language.object.liquid or support.type.object.liquid on expression BUT when another type is referenced, like a string, number or boolean then the appropriate scopes for those types are applied.

This way the syntax colours are cosmic and well inferred but not to a point of unreadability. I suppose there is a fine line there. Essentially, this pattern to follow suite:

This feels the most logical step forward and will solve the issue while still adding extra flavour.

panoply avatar Sep 29 '22 21:09 panoply

@panoply Agree! I like both options but true, if there's a big deep nested object it can get very colorful and crazy haha so going with the second option only highlighting the lasts 2 values seems great ! 👏🏼

muchisx avatar Sep 29 '22 22:09 muchisx

@muchisx I will schedule this for v3.1.0 as a lot has changed and it calls for a minor. I'd like to iron out some other defects pertaining to formatting before merging. I was able to cover all the cases discussed. The new grammars apply highlighting on a type basis. The syntax support that will be employed in this extension as of v3.1.0 will hopefully solidify it as the breadwinner and preferred choice over the Shopify Liquid extension grammars.

Big thanks to @muchisx for his groundwork here.

Liquid Tag

All fixes and improvements have been applied to the Liquid tag

Screenshot 2022-09-30 at 09 17 49

Liquid Objects

As per our discussion, I have applied typed scopes for objects. I also inferred entity.tag.* scopes for constant type object tags, like {{ content_for_header }}. I was able to ensure various expressions passed, like (for example) dashed unquoted props, eg: some-object.prop, additional word sequences and character, eg _100.prop and much more!

Screenshot 2022-09-30 at 09 18 20

Liquid logicals

Instead of inferring operator scopes on in, with and as these will instead use an entity.* type scope. This is just far more cleaner overall and provides better targeting for token customisations.

Screenshot 2022-09-30 at 09 32 00

panoply avatar Sep 30 '22 07:09 panoply

Shipped https://github.com/panoply/vscode-liquid/pull/110

Big thanks to @muchisx

panoply avatar Oct 25 '22 04:10 panoply