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

feat: support keyword arguments with no definition on custom filters #516

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

Conversation

juanazam
Copy link
Contributor

  • Tests created for any new feature or regression tests for bugfixes.

Implements the changes required to support what's described on #507.
In order to support arbitrary keyword arguments, this PR introduces a new type of parameter.

When defined, the keyword_group parameter is populated with all keyword parameters used to call the filter.
It also disables the validation of only allowing known keyword parameters to be used on custom filters.

It also introduces a new variant for Expression (defined at runtime) to support this.

I can update this pr in any way maintainers feel it makes more sense.

@juanazam juanazam force-pushed the support_keyword_group_filter_parameter branch 2 times, most recently from 51c048c to dd97d2f Compare September 18, 2023 13:45
@juanazam juanazam force-pushed the support_keyword_group_filter_parameter branch from dd97d2f to e597967 Compare September 18, 2023 13:46
@epage
Copy link
Member

epage commented Sep 25, 2023

This feels very hacked-in. Any idea how this is implemented with the ruby implementation? Its been a while since I've looked but my guess is they expose raw text for it to parse as they want rather than having any kind of higher level constructs to work with.

@juanazam
Copy link
Contributor Author

juanazam commented Sep 26, 2023

Hi @epage

Thanks for the feedback.
The ruby implementation is quite simple, because given it’s dynamic, they translation filter receives the translation key and the variables as a hash.

Here is the implementation: https://github.com/Shopify/liquid/blob/0b9318222bcc09681e52fd5b8e70262274e673bf/lib/liquid/i18n.rb#L17
And here an example on a test: https://github.com/Shopify/liquid/blob/0b9318222bcc09681e52fd5b8e70262274e673bf/test/unit/i18n_unit_test.rb#L21
The fixture file is here: https://github.com/Shopify/liquid/blob/0b9318222bcc09681e52fd5b8e70262274e673bf/test/fixtures/en_locale.yml

The translation fixture file contains:

---
  simple: "less is more"
  whatever: "something %{something}"

The call on the test @i18n.translate("whatever", something: "different”) will replace the usage of something with the something variables passed as argument on the filter.

In the case of rust-liquid, I couldn’t find another workaround on how to indicate that I want my custom filter to receive any arbitrary variables and not have to define them when defining the filter. This is necessary because liquid users can pass any variables to the filter and we cannot know them in advance.

I will be glad to update this PR in any direction that feels better, but I’m not sure what’s the best way besides the one that I found.

@juanazam
Copy link
Contributor Author

Hi @epage, did you have a chance to see this? thanks!

@juanazam
Copy link
Contributor Author

Hey @epage any comments/advice on what can be done here?

@juanazam
Copy link
Contributor Author

Hi @epage, any suggestions here on how to handle this?

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.

2 participants