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

New UnusedDocParam theme-check rule #808

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

aswamy
Copy link
Contributor

@aswamy aswamy commented Feb 21, 2025

What are you adding in this PR?

Closes #803

  • Add check to ensure @param variable is actually used in the snippet

Questions

Why didn't i update UnusedAssign as well to ignore loop-scoped variables

UnusedAssign can give false-positive if we also ignore loop-scoped variables because of situations like this:

{% assign foo = 'example' %} // Would have UnusedAssign variable: GOOD

{% for foo in array %}
  {% assign foo = 'override' %} // Would have UnusedAssign variable: BAD
  {{ foo }}
{% endfor %}
  • We would have to maintain a list of all variables and their scopes, which wouldn't have a good effort/value ratio

Tophat

  • Have a snippet with doc
  • Ensure the params defined are actually used
  • We will consider it "unused" if the variable is actually a for-loop scoped variable
  • You can hover over the warning, and press quick fix to remove the param

e.g.

{% doc %}
  @param {string} name - This is a name.
  @param {object} opts - options // Unused
  @param {unknown} extra - extra data // Unused
{% enddoc %}

{{ name }}

{% tablerow extra in product.variants %}
  {{ extra }}
{% endtablerow %}

{% for extra in product.variants %}
  {{ extra }}
{% endfor %}

Before you deploy

  • I included a minor bump changeset
  • My feature is backward compatible

@aswamy aswamy requested a review from a team as a code owner February 21, 2025 17:26
@aswamy aswamy added the #gsd:44310 LiquidDoc label Feb 21, 2025
Copy link
Contributor

Left a couple of nits but functionally ✅

@aswamy aswamy requested a review from jamesmengo February 24, 2025 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:44310 LiquidDoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Stretch) Update the theme-tools linter to present a warning when the header has unused parameters
2 participants