Skip to content

Liquid Support #3322

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

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

Liquid Support #3322

wants to merge 38 commits into from

Conversation

3limin4t0r
Copy link

I use the liquid editor, but needed more liquid support.

Previously liquid had one mode supporting HTML and Liquid highlighting, but this mode had no HTML/XML tag completion.

I stripped Liquid in two parts, around the same way php was done. The default Liquid mode now only support plain/text. However, I added two new modes: HTML (Liquid) & YAML (Liquid). Since I need Liquid in plain text, HTML and YAML.

I've also added some TODOs for the things I don't have time for right now, but might need attention in the future.

I've requested the CLA, but don't have a response yet. This will be attached at a later point.

@nightwing
Copy link
Member

@3limin4t0r is there some documentation about yaml and txt liquid modes, and is the plain text version used anywhere? Changing the liquid mode like this is a breaking change which we should make only if absolutely necessary

@3limin4t0r
Copy link
Author

@nightwing liquid is like erb or php an overlay which gets parsed before it goes through the other parser. So I don't see why the default shouldn't be text. I can change it back, so the default is html, but if people want to use it for plain text there is not really an option in that case.

@nightwing
Copy link
Member

I've requested the CLA, but don't have a response yet. This will be attached at a later point.

What did you mean, from whom did you request the CLA?

So I don't see why the default shouldn't be text.

This depends on behavior of other text-editors, and on the way .liquid files are usually used, but i couldn't find much info about this, do you know any open source projects that have .liquid files?

but if people want to use it for plain text there is not really an option in that case.

We could support .txt.liquid for that case

@3limin4t0r
Copy link
Author

3limin4t0r commented Aug 21, 2017

What did you mean, from whom did you request the CLA?

We followed the corporate link on the CONTRIBUTING.md page and filled in the form. But haven't heart anything back. If needed we can submit the info a second time.

This depends on behavior of other text-editors, and on the way .liquid files are usually used, but i couldn't find much info about this, do you know any open source projects that have .liquid files?

I don't know if any open source project use it for plain/text usage. However, I may need this functionality in the future for the project I'm currently working on (although not open source). It doesn't have to be the default. So I've no problem switching back to the HTML default, since that's the most used format.

We could support .txt.liquid for that case

Would it be okay if I rewrote Liquid to default to HTML and make specific versions for text and YAML? Resulting in the modes liquid (HTML), yaml_liquid (YAML) and plain_text_liquid (plain text) instead of liquid (plain text), html_liquid (HTML) and yaml_liquid (YAML).

@xhoy
Copy link

xhoy commented Jun 12, 2018

@nightwing can this be merged??!?!?!

@nudded
Copy link

nudded commented Aug 16, 2018

@3limin4t0r any chance you can fix the conflicts? I'm also very interested in this.

@3limin4t0r
Copy link
Author

@nudded Not on short term. However conflicts need to be resolved anyway, since we use this version in production and are otherwise stuck with the old version of the ace editor. I'll check with my employer if I can reserve a day to resolve the conflicts somewhere in the coming few weeks.

@3limin4t0r
Copy link
Author

@nudded I merged the PR with upstream master and should be up to date with the latest ace version (a7acb59).

@nudded
Copy link

nudded commented Feb 15, 2019

@nightwing Any further action that needs to be taken here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants