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.

Sign up for GitHub

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

Jump to bottom

[LiquidDoc][Theme Check] Check for duplicated params passed to {% render %} tag #834

Merged
merged 2 commits into from
Mar 1, 2025
Merged

[LiquidDoc][Theme Check] Check for duplicated params passed to {% render %} tag #834

merged 2 commits into from
Mar 1, 2025

Conversation

jamesmengo
Copy link
Contributor

@jamesmengo jamesmengo commented Feb 28, 2025 •
edited
Loading

What are you adding in this PR?

Closes https://github.com/Shopify/developer-tools-team/issues/539

Add a theme check to check for duplicated param passed into a {% render %} tag

Includes:

  • Reporting duplicated params
  • Suggesting fixes to remove -> We will always keep the first occurrence and suggest removal for subsequent occurrences
  • Support for aliases {% render 'snippet-name' with 'test' as param, param: 'test' %}

Before you deploy

  • This PR includes a new checks or changes the configuration of a check
    • I included a minor bump changeset
    • It's in the allChecks array in src/checks/index.ts
    • I ran yarn build and committed the updated configuration files
      • If applicable, I've updated the theme-app-extension.yml config
    • I've made a PR to update the shopify.dev theme check docs if applicable (link PR here).

@jamesmengo Graphite App
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jamesmengo jamesmengo changed the title Add DuplicateRenderSnippetParams theme check [LiquidDoc][Theme Check] Check for duplicated params passed to {% render %} tag Feb 28, 2025
@jamesmengo jamesmengo added the #gsd:44310 LiquidDoc label Feb 28, 2025
@jamesmengo jamesmengo requested a review from aswamy February 28, 2025 02:10
@jamesmengo jamesmengo marked this pull request as ready for review February 28, 2025 02:10
@jamesmengo jamesmengo requested a review from a team as a code owner February 28, 2025 02:10
jamesmengo
jamesmengo commented Feb 28, 2025
View reviewed changes
packages/theme-check-common/src/checks/duplicate-render-snippet-params/index.ts
suggest: [
{
message: `Remove duplicate parameter '${paramName}'`,
fix: (fixer) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part has me feeling icky, I haven't landed on an approach yet. I'm considering many.
I want to be pragmatic here. I don't think this code will change soon, and if it does I'm hoping that we can simplify this logic by potentially getting more information during the parse step instead.

Not sure how much time I want to invest as a result.

Ideas

  • Leaving this comment here to indicate duplication
  • Extracting to a shared utility but testing in the tests for individual checks
  • Extracting a utility that just provides indexes

Factors

  • The code is 1-1 duplicated, so I do want to call out duplication
  • This code IS tested through the individual theme checks.
  • I tried extracting this to a utility and didn't like the way I had to write tests. Probably a skill issue. Will take time either way.
  • I feel like the better approach is to do this programatically via modifying parsing logic.

@aswamy WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paired on this. Not ideal, but we're going to leave this as is as it's the more pragmatic approach.

We should reconsider the next time we touch this code

aswamy
aswamy approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@aswamy aswamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor changes

@jamesmengo
Add DuplicateRenderSnippetParams theme check
a851f37 
Introduces a new theme check to detect and report duplicate parameters in Liquid render tags. The check:
- Identifies duplicate parameter names in render snippets
- Provides suggestions to remove redundant parameters
@jamesmengo
PR Feedback
a3d9762 
- Extract `isLastParam` function to shared utility
- Improve source slicing and comma matching for parameter removal
- Ensure consistent parameter removal across different checks
@jamesmengo Graphite App
Copy link
Contributor Author

jamesmengo commented Mar 1, 2025 •
edited
Loading

Merge activity

  • Feb 28, 10:12 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Feb 28, 10:12 PM EST: A user merged this pull request with Graphite.

@jamesmengo jamesmengo merged commit 2483c3c into main Mar 1, 2025
7 checks passed
@jamesmengo jamesmengo deleted the jm/dup_params branch March 1, 2025 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aswamy aswamy

Assignees
No one assigned
Labels
#gsd:44310 LiquidDoc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jamesmengo @aswamy