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

Added filter to apply dynamic filters #203

Merged
merged 9 commits into from
Oct 1, 2018
Merged

Added filter to apply dynamic filters #203

merged 9 commits into from
Oct 1, 2018

Conversation

ilyapuchka
Copy link
Collaborator

filter:arg will resolve its argument as filter expression which will allow to apply dynamic filters.

The use case for that can be using Sourcery to generated code for Codable protocols and use different key coding strategies applied for different data models. It will allow to specify strategy as one of Stencil filters in a type annotation, i.e. uppercaseFirstLetter or camelCaseToSnakeCase, or even filter with arguments.

Currently in template I have to check annotations against hardcoded set of filters to be able to apply them. With this change this template code

{% if strategy == "upperFirstLetter" %}{{ variable.name|replace:"_",""|upperFirstLetter }}{% else %}{{ variable.name|replace:"_",""|camelToSnakeCase }}{% endif %}

becomes this

{{ variable.name|replace:"_",""|filter:strategy }}

and allows to use any registered filter without changing anything in template.

@AliSoftware
Copy link
Collaborator

I'll have to re-check the Stencil docs (on phone rn so not the best device to read them) but I thought Stencil already had a {% filter xx %}…{% endfilter %} tag that would also allow you to do dynamic filters already?

@AliSoftware
Copy link
Collaborator

My bad, nvm, that tag indeed already exists but expects a filter name not a variable so isn't able to do dynamic filters. Forget I said anything

@ilyapuchka
Copy link
Collaborator Author

ilyapuchka commented May 10, 2018

@AliSoftware the implementation of filter node only converts the whole node to filter expression applied to the rendered content of the node, it does not resolve the name of the filter as variable. But that's something we can add too! Also I just find filter expression easier to use than filter node and better composable (chaining vs nesting)

CHANGELOG.md Outdated
@@ -12,6 +12,8 @@
[Ilya Puchka](https://github.com/yonaskolb)
[#178](https://github.com/stencilproject/Stencil/pull/178)

- Added support for dynamic filter using `filter` filter
Copy link
Contributor

@djbe djbe Jul 11, 2018

Choose a reason for hiding this comment

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

If (when?) #221 gets merged (merged), could you fix the format of this entry? Remove the newline above, add a . and 2 spaces at the end, and credit yourself + link to this PR.

@@ -71,8 +71,24 @@ public class TokenParser {
tokens.insert(token, at: 0)
}

public func compileFilter(_ token: String) throws -> Resolvable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these methods (compileFilter, compileExpression and compileResolvable) have to be public?

Note: if they can be internal, don't forget to reset the Expression protocol back to internal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So that they can be used by 3rd party extensions

Copy link
Contributor

Choose a reason for hiding this comment

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

Ehm, do they need to be used by 3rd party extensions? Do you have a scenario where that'd be necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I cant think of any now but if I needed it in filter then it might be needed in something else complex too. Anyway, I don't see any harm in making these methods public.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, a library should only expose the minimal surface necessary for a developer to use it. If at any point, something is missing, it can be added later on.

Maybe someone else has some thoughts on this point? @AliSoftware?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd normally agree with limiting the surface of the public API, but given all other stuff like parse(until:), nextToken() and prependToken are already public too, I say it's fine by me.

TBH I haven't used Stencil as a library to be extended for a while now (only use case for us is StencilSwiftKit to extend Stencil), not sure there are many people using those features — main uses of this lib is probably just using it as-is to render templates, adding maybe some custom filters and that's all), that shouldn't really bother them.

BUT, if we're gonna have so much public API, then we should then definitely add doc-comments to them to explain to end users what they're supposed to be used for (especially to those not completely familiar with the concepts of a parser vs lexer etc, and proper definition of token etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea on the docs then. It took me quite a while to understand the codebase a bit, and the documentation in the code is quite sparse.

Sources/Parser.swift Show resolved Hide resolved
Sources/Extension.swift Show resolved Hide resolved
@djbe
Copy link
Contributor

djbe commented Jul 11, 2018

Except from the small changelog change, and some small access questions, this LGTM.

@djbe
Copy link
Contributor

djbe commented Jul 11, 2018

Before I forget: This PR should document the changes in the docs.

@djbe djbe added this to the 0.12.0 milestone Jul 11, 2018
@ilyapuchka
Copy link
Collaborator Author

@djbe comments addressed

``filter``
~~~~~~~~~

Applies filter by its name, provided as an argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Applies the filter with the name (provided as argument) to the current expression.


{{ string|filter:myfilter }}

This expression will resolve `myfilter` variable and will apply filter with such name to `string` variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

This expression will resolve the myfilter variable, and it will apply the filter with that resolved name to the string variable.

@djbe
Copy link
Contributor

djbe commented Sep 11, 2018

This will need a rebase, based on 0.12. Be careful when rebasing the changelog, as the entry will need to be moved to the right section (under master).

@djbe djbe modified the milestones: 0.12.0, 0.13.0, 0.14.0 Sep 21, 2018
# Conflicts:
#	CHANGELOG.md
#	Sources/ForTag.swift
#	Sources/IfTag.swift
#	Sources/Parser.swift
#	Sources/Variable.swift
#	Tests/StencilTests/ExpressionSpec.swift
#	Tests/StencilTests/FilterSpec.swift
#	Tests/StencilTests/ForNodeSpec.swift
#	Tests/StencilTests/VariableSpec.swift
@ilyapuchka
Copy link
Collaborator Author

@djbe I updated docs. We'll need this, in particular making Expression type public to add support for {% call this if condition else that %} in StencilSwiftKit, unless we move call node into Stencil.

@djbe
Copy link
Contributor

djbe commented Oct 1, 2018

Looks GTM. How come this branch is till out-of-date after that merge you did one hour ago?

@ilyapuchka
Copy link
Collaborator Author

🤷‍♂️

@djbe djbe merged commit 6f3ca60 into master Oct 1, 2018
@djbe djbe deleted the dynamic-filter branch October 1, 2018 22:50
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