Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Enhance auto-closing pairs and auto-surrounding pairs #115

Closed

Conversation

timdmackey
Copy link

@timdmackey timdmackey commented Mar 8, 2023

This PR improves on the behaviour of auto-closing pairs and auto-surrounding pairs. These changes follow the rules I developed for my Liquid for Mechanic extension, which I've used extensively for my own work. I've found the enhancements to be quite useful.

Additions to current syntax:

  • adds auto-closing support for - whitespace control characters inside of liquid tags
  • adds auto-closing support for padding spaces inside of liquid tags
  • adds auto-closing support for padding spaces inside of comment tags
  • adds auto-closing support for [ brackets, < parentheses, { braces, and < angle brackets
  • adds auto-surrounding support for wrapping text with spaces
  • adds auto-surrounding support for wrapping text with the % symbol (for wrapping a string into a new tag)
  • removes redundant auto-closing declaration for {{ double braces
  • standardizes auto-closing rules to close with one character

Caveats:

  • When backspacing to delete an opening bracket, quote, etc, it's often the case that the matching closing token will be deleted too. However, this only works with single character pair definitions (this is a limitation of Textmate grammars). The result being: If you type a complex opening pattern like {%, resulting in {%‸%}, when you hit backspace to delete the % it will leave the closing % in place (this is already the case for the current version of this extension). The same goes for the dashes and spaces I've added in this patch.
  • The solution to the above issue for spaces, - whitespace control, and the % symbol is to define these as single-character auto-closing pairs. However, spaces are far too common of a character, meaning you end up with extra spaces popping up willy-nilly. % is less common, but it's still common enough that it's confusing if an isolated % symbol gets automatically duplicated. For these reasons, I decided the broken backspace behaviour is preferable.

Additional Notes:

  • Line 34 was removed because it is redundant. Line 33 performs the same auto-completion function, and backspacing to remove the opening and closing pair only works with a single character.
  • I standardized the auto-closing rules to close with one character, because otherwise some of the rules will conflict and create extraneous characters. For example, using {{ and }} as a pair results in 3 closing braces, due to conflict with the single brace auto-closing rule.
  • The one rule I think could be contentious is enabling the % symbol as an auto-surrounding pair. In my opinion (and experience with the above-mentioned extension), needing to select and replace any text with a % is so rare that this possibly unintended wrapping behaviour is not disruptive.

@timdmackey timdmackey force-pushed the auto-closing-pairs-enhancement branch 2 times, most recently from 1b542ae to e8d0b47 Compare March 8, 2023 08:20
@timdmackey
Copy link
Author

I just noticed commit 339744a, which removed support for whitespace autocompletion, partially to fix the issue presented in #48. However, this issue was actually caused by the fact that the auto-closing pair rules had multiple characters for the second half of the pair. As noted in my notes above, the second part of these pairs should always be a single character. Converting all of the rules to close with a single character fixes issue #48 without removing the ability to auto-close whitespace control characters.

@timdmackey
Copy link
Author

timdmackey commented Mar 10, 2023

On further thought, it may be a good decision that whitespace control characters don't auto-close. The reason being, that it's often the case you only want to apply whitespace control to one side of the tag (mentioned by @charlespwd here).

Also, this PR doesn't entirely fix the casae described in #48. It just means if you manually type a - into an already created tag, it will create a second dash rather than also creating extra braces: {{-- }}

So, I'm somewhat inclined to retract my request for whitespace control auto-closure, but to retain my request for whitespace auto-closure (including auto-closing whitespace after a dash).

@charlespwd
Copy link
Contributor

charlespwd commented Oct 30, 2023

Yeah I'm trying to revisit this again now that things are clearing up in Shopify/theme-tools (this repo is moving there). Tried it, getting the same issue where even just adding a space on one end closes the tag again.

{% assign x = 1 %}
  ^ add space here
{%  %}assign x = 1 %}

I started looking into the source code and the logic that they use is like this (if I understand correctly):
canAutoclose if:

  • nextCharacter is one of the characters in the autoCloseBefore character set
  • nextCharacter is not one of the characters in the closingPairs for that character pair

And we're always going to be screwed because of the space I think. I think we're going to take a Language Server approach for this. There's doesn't seem to be a valid way with languageConfiguration.

@mgmanzella
Copy link

👋 this repo is no longer the source of truth for theme check, if youd still like to make this change please open a new PR in the theme tools repo 🙏 thank you!

@mgmanzella mgmanzella closed this Jun 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants