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

ConvertToInclude autocorrection breaks the code if snippet uses variables defined in parent context #445

Open
charlespwd opened this issue Sep 14, 2021 · 0 comments
Labels
bug Something isn't working e:5 Effort: 5 p:low Priority (or Impact): Low
Projects

Comments

@charlespwd
Copy link
Contributor

charlespwd commented Sep 14, 2021

The ConvertIncludeToRender autocorrector doesn't correct properly if the snippet assumes a variable was defined in its parent context.

Example:

// templates/product.liquid
{% assign x = "hello world" %}
{% include 'echo-x' %}

// snippets/echo-x.liquid
{{ x }}

// rendered content
hello world

The autocorrector would flip that to this:

// templates/product.liquid
{% assign x = "hello world" %}
{% render 'echo-x' %}

// snippets/echo-x.liquid
{{ x }}

// rendered content
Undefined Variable 'x'

What it should do instead:

{% render "echo-x", x: x %}

Proposed solution / thoughts

  1. Gather all the UndefinedAssigns in the snippet that the include refers.
  2. Pass those as arguments to the render when correcting.

Note:

I feel like we should disable this autocorrection until this is fixed. Breaking a site on first autocorrect doesn't seem like the way to go.

@charlespwd charlespwd added bug Something isn't working e:5 Effort: 5 p:medium Priority (or Impact): Medium labels Sep 14, 2021
@charlespwd charlespwd changed the title ConvertToInclude autocorrection breaks the code if snippet uses variables defined in Context ConvertToInclude autocorrection breaks the code if snippet uses variables defined in parent context Sep 14, 2021
@charlespwd charlespwd added this to Needs triage in Maintenance via automation Sep 14, 2021
@charlespwd charlespwd added p:low Priority (or Impact): Low and removed p:medium Priority (or Impact): Medium labels Sep 14, 2021
@charlespwd charlespwd moved this from Needs triage to Low priority in Maintenance Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working e:5 Effort: 5 p:low Priority (or Impact): Low
Projects
No open projects
Maintenance
Low priority
Development

No branches or pull requests

1 participant