Skip to content

Conversation

@jaime-amate
Copy link

@jaime-amate jaime-amate commented Nov 18, 2025

The aim of this PR is to add a function that allows to remove a defined tag. This is useful for my specific need as I need to disable a standard tag ({% include %}) and still get a parse error.

Currently in order to disable a standard tag, you can:

// Register a tag that always returns an error
engine.RegisterTag("include", func(c render.Context) (string, error) {
    return "", fmt.Errorf("Tag include is not allowed")
})

This overrides the existing tag functionaly of include and will throw an error at render time. Adding RemoveTag allows to remove standard tags dinamically.

Checklist

  • I have read the contribution guidelines.
  • make test passes.
  • make lint passes.
  • New and changed code is covered by tests.
  • Performance improvements include benchmarks.
  • Changes match the documented (not just the implemented) behavior of Shopify.

@jaime-amate jaime-amate marked this pull request as ready for review November 18, 2025 15:24
@osteele osteele requested review from Copilot and osteele November 28, 2025 23:26
Copilot finished reviewing on behalf of osteele November 28, 2025 23:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a RemoveTag method to the Engine API, enabling dynamic removal of tag definitions from the engine configuration. This addresses the use case of disabling standard tags (like {% include %}) and getting parse-time errors instead of render-time errors when those tags are used.

Key changes:

  • Added RemoveTag method to render.Config that uses Go's delete function to remove tags from the internal map
  • Exposed RemoveTag through the public Engine API with comprehensive documentation
  • Added test coverage verifying that removed tags cause parse/render errors

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
render/tags.go Implements low-level RemoveTag method on Config that deletes tag from internal map
engine.go Exposes public RemoveTag API with clear documentation about idempotent behavior
engine_examples_test.go Adds test verifying custom tag removal causes errors on subsequent use

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment on lines +309 to +323
func TestRemoveTag(t *testing.T) {
engine := NewEngine()
engine.RegisterTag("echo", func(c render.Context) (string, error) {
return c.TagArgs(), nil
})
source := `{% echo hello world %}`

_, err := engine.ParseAndRenderString(source, emptyBindings)
require.NoError(t, err)

engine.RemoveTag("echo")

_, err = engine.ParseAndRenderString(source, emptyBindings)
require.Error(t, err)
}
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The test only verifies removing a custom tag (echo), but the PR description specifically mentions removing standard tags like {% include %} to get parse errors. Consider adding a test case that removes a standard tag to ensure this use case works as intended.

Example:

t.Run("RemoveStandardTag", func(t *testing.T) {
    engine := NewEngine()
    source := `{% if true %}test{% endif %}`
    
    _, err := engine.ParseAndRenderString(source, emptyBindings)
    require.NoError(t, err)
    
    engine.RemoveTag("if")
    
    _, err = engine.ParseAndRenderString(source, emptyBindings)
    require.Error(t, err)
})

Copilot uses AI. Check for mistakes.
}

// RemoveTag removes a tag definition.
func (c *Config) RemoveTag(name string) {
Copy link
Owner

Choose a reason for hiding this comment

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

Consider whether this should be RemoveTag or UnregisterTag. The first reads better in isolation; the second has better symmetry with RegisterTag.

This is just a question for you, not a requirement for me to accept the PR. I'm okay with either. Don't do both though! This ends up making it harder to work with client code.

})
}

func TestRemoveTag(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

As a test, this should go in engine_test.go. Or rename it to ExampleEngine_RemoveTag as a testable example.

Copy link
Owner

@osteele osteele left a comment

Choose a reason for hiding this comment

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

Thank you for contribution! This looks solid. A couple of minor review comments are attached.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants