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

Theme check for settings keys inside presets and default #742

Merged
merged 2 commits into from
Feb 10, 2025

Conversation

aswamy
Copy link
Contributor

@aswamy aswamy commented Jan 29, 2025

What are you adding in this PR?

Fixes #624
Fixes #602

  • Talked to @charlespwd and we won't be doing setting value check here

Shoutout to @AribaRajput for creating the initial PR to get this done quickly

What's next? Any followup issues?

  • Found a few missing pieces of completion / hover / checks for schema that ill be tackling

How to test

NOTE: This check DOES NOT check if the theme block exists or not. That is the responsibility of another theme check. I have noticed a bug where if you have local blocks, and there are references to local blocks that dont exist within presets and default, no theme check error is thrown. I will fix that in a following PR.

Testing settings

  1. Create preset/default settings
"settings": [{
    "type": "text",
    "id": "top-level-setting",
    "label": "TEXT"
  }
],
  1. Create presets.[].settings and default.settings that has keys that match the settings.id defined above. Create some that don't. See error.

Testing preset/default block settings defined in local block

  1. Create a local block with settings
"blocks": [
  {
    "type": "local-block",
    "name": "local block",
    "settings": [
      {
        "id": "local-setting",
        "type": "text",
        "label": "local setting"
      }
    ]
  }
]
  1. Create presets.[].blocks.[].settings and default.blocks.[].settings that has keys that match the settings.id defined above. Create some that don't. See error.

Testing preset/default block settings defined in theme block

  1. Create blocks that allows all theme blocks
"blocks": [
  {
    "type": "@theme"
  }
]
  1. Create presets.[].blocks.[].settings and default.blocks.[].settings that has keys that match the settings.id defined in the block file being referenced. Create some that don't. See error.

Before you deploy

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

Sorry, something went wrong.

expect(offenses[0].message).to.equal(
`Setting 'non-existent-setting' does not exist in schema.`,
);
});
Copy link
Contributor

@jamesmengo jamesmengo Feb 7, 2025

Choose a reason for hiding this comment

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

Let's add a case for n>1 offenses + a case where we have 1 missing 1 valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can definitely do 2 missing settings. But checking across multiple rules wouldn't be unit testing this rule.

Copy link
Contributor

@jamesmengo jamesmengo left a comment

Choose a reason for hiding this comment

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

Review went well, and the only edge case I could find is when referencing a block with theme check errors (this doesn't render anything, which is expected - in this screenshot, menu has an invalid schema)

I wonder if we could report invalid schemas 🤔

Either way, this changeset LGTM!

image.png

@aswamy
Copy link
Contributor Author

aswamy commented Feb 10, 2025

the only edge case I could find is when referencing a block with theme check errors (this doesn't render anything, which is expected

Yes this is known. Unfortunately, we have to be more lenient with our theme checks - which unfortunately makes theme check worse 😅. We could still try to do it, but seems on-par with a lot of other languages where you have syntax errors in referenced files.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@aswamy aswamy force-pushed the aswamy/presets-theme-check branch from b696ba6 to 65f9c9d Compare February 10, 2025 15:01
@aswamy aswamy merged commit 055cef7 into main Feb 10, 2025
6 checks passed
@aswamy aswamy deleted the aswamy/presets-theme-check branch February 10, 2025 15:11
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.

Theme block JSON preset validation Validate preset settings
3 participants