Shopify / theme-check

The Ultimate Shopify Theme Linter
Other
333 stars 96 forks source link

Suggest filters compatible with the object type #669

Closed Poitrin closed 1 year ago

Poitrin commented 1 year ago

WHY are these changes introduced?

Fixes #658

WHAT is this pull request doing?

  1. Adds logic to FilterCompletionProvider to…

    • determine the type of the variable/literal (string, number, array, …) sitting before the filter separator ("input type")
    • suggest filters that match the specific input type, and filters whose input type is variable (i.e. for more than 1 specific type, e.g. | default)
  2. Displays deprecated filters too, so that users can still see the filter they wanted to use.

How to test your changes?

For all cases (if not stated differently):

array

{% assign ct = current_tags | 
{% assign c = product.collections | 
{{ current_tags | 
{{ product.collections | 

{% assign ct = current_tags %}
{{ ct | 

{{ blog.metafields | 

string

{% assign t = product.title | 
{{ page_description | 

{% assign t = product.title %}
{{ t | 

{{ 'test%40test.com' | 
{{ '' | 

{% for tag in collection.all_tags %}
  {%- if current_tags contains tag -%}
    {{ tag | 

{{ product.selected_variant.url | 

{%- assign collection_product = collection.products.first -%}
{{ collection_product.url | 

{% assign secret_potion = 'Polyjuice' | 

{%- assign text = '  Some potions create whitespace.      ' -%}
{{ text | 

number

{{ cart.total_price | 

{% assign tp = cart.total_price %}
{{ tp | 

{{ -4.2 | 

form

{{ form | 

font

{{ font.variants.first | 

media

{{ product.featured_media | 

{% for media in product.media %}
  {% if media.media_type == 'external_video' %}
    {% if media.host == 'youtube' %}
      {{ media | 

metafield

{{ shop.metafields | 

paginate

{{- paginate | 

address

{{ shop.address | 

Display all filters

In the following examples, return types have no corresponding filter (e.g. article.image has return type image, but there is no filter for image |). Therefore, make sure that all filters are suggested.

{{ article.image | 
{{ product.metafields.information.seasonal | 
{{ product | 
{{ settings.type_header_font | 
{{ false | 

{%- assign display_price = false -%}
{{ display_price | 

Display deprecated filters

{{ 'foo.js' | hex_to
{% assign t = product.title | 

Verify that deprecated filters are crossed out, and that a deprecation reason is shown. deprecation

mgmanzella commented 1 year ago

should i be seeing these deprecated filters for integers and other object types?

Screenshot 2022-12-08 at 12 25 09 PM Screenshot 2022-12-08 at 12 23 02 PM
Poitrin commented 1 year ago

should i be seeing these deprecated filters for integers and other object types?

Unfortunately yes – I did not change the logic, the deprecated filters are just more prominent now 😢: If I take article_img_url as an example, you will see that its input type is variable:

  "deprecated": true,
  "syntax": "variable | article_img_url",
  "name": "article_img_url"

Therefore, these filters – no matter if they are deprecated or not – are suggested for every type.

Among 9 variable filters, 4 are deprecated (which are the ones you see in your screenshots).

start = { 'variable' => [0, 0], 'string' => [0, 0], 'form' => [0, 0], 'font' => [0, 0],
'number' => [0, 0], 'array' => [0, 0], 'media' => [0, 0], 'metafield' => [0, 0], 'paginate' => [0, 0], 'address' => [0, 0], 'type' => [0, 0] }
ShopifyLiquid::SourceIndex.filters.each_with_object(start) do |filter, result|
  result[filter.input_type][filter.deprecated? ? 1 : 0] += 1
end          

=> {"variable"=>[5, 4], "string"=>[82, 2], "form"=>[2, 1], "font"=>[3, 0], "number"=>[17, 0], "array"=>[11, 0], "media"=>[4, 0], "metafield"=>[2, 0], "paginate"=>[1, 0], "address"=>[1, 0], "type"=>[2, 0]}

Possible solutions

  1. (my fav) We update the docs so that the 4 aforementioned deprecated filters get the input type image instead. That way, they are only suggested when working with images like article.image.
  2. We hide all deprecated filters. (Or at least deprecated variable filters.)
mgmanzella commented 1 year ago

Unfortunately yes – I did not change the logic, the deprecated filters are just more prominent now 😢:

goooot it thanks for the explanation! i think if you havent already lets capture this in an issue as a possible enhancement. for now tho i think it's a good idea to keep it as is, we can get feedback from developers what we release and see if it's confusing. but i suspect hiding the deprecated suggestion all together will be more frustrating

Possible solutions (my fav) We update the docs so that the 4 aforementioned deprecated filters get the input type image instead. That way, they are only suggested when working with images like article.image. We hide all deprecated filters. (Or at least deprecated variable filters.)

sorting deprecated variable suggestions to the bottom might also be a good compromise 😮

Poitrin commented 1 year ago

if you havent already lets capture this in an issue as a possible enhancement

https://github.com/Shopify/theme-check/issues/676Done :-)