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

Additional boolean templates; #69

Merged
merged 1 commit into from
Nov 20, 2017
Merged

Conversation

Antondomashnev
Copy link
Contributor

This is a result of the work on the Sourcery task. It brings the negative counterpart for Strings+Boolean filters:

  • !contains
  • !hasPrefix
  • !hasSuffix

@AliSoftware @djbe I'm not sure whether we need to update docs or not, from my point of view we don't 😄

@djbe
Copy link
Member

djbe commented Oct 5, 2017

Hmmm, why use these !stuff instead of not stuff? Conciseness?

Do we know if Kyle is planning on adding parsing for a shorthand not?

@Antondomashnev
Copy link
Contributor Author

Antondomashnev commented Oct 5, 2017

@djbe I didn't ask @kylef, maybe it would be a good addition for the Stencil itself.

Hmmm, why use these !stuff instead of not stuff? Conciseness?

I think it's a handy shorthand and I'd be honest one more reason is to simplify removing of filters from Sourcery that are now here 😄

@AliSoftware
Copy link
Contributor

I think we should wait for @kylef to chime on this one. Not sure adding a ! counterpart instead of just using not is really worth it, especially if it's only applied to some filters (the 3 string boolean filters StencilSwiftKit adds here) and not all (all boolean filters Stencil proper declares)

@kylef
Copy link

kylef commented Oct 9, 2017

I am wondering what the use-case is here. Would they be used in an if statement? Which there is already a not.

Thanks to stencilproject/Stencil#143 there is an in expression:

{% if "thing" not in collection %}
{% endif %}

(I don't recall if filters are supported in if statements, but something like the following may work or be easy to implement):

{% if not collection|hasPrefix:"test" %}
{% endif %}

@Antondomashnev
Copy link
Contributor Author

Antondomashnev commented Oct 11, 2017

@kylef @AliSoftware to be honest, the most reason is to be able to easily remove filters from Sourcery that are now in StencilSwiftKit.
The link to these three filters is here and as you may find the registerBoolFilterOrWithArguments generates also !{filter}.

Generally speaking I'm fine to not merge this PR as it's actually not as generic as it should be especially with not keyword in Stencil.

@krzysztofzablocki
Copy link
Contributor

krzysztofzablocki commented Nov 13, 2017

not operator does not allow for chaining e.g. in Sourcery we use stuff like
{% for type in types.all|!inheriting.BaseClass|!annotated:skipAutomaticDecoding %}

@AliSoftware
Copy link
Contributor

Again, my only concern is that some boolean filters (the ones in Stencil proper) will not have this kind of ! operators while the ones declared in StencilSwiftKit will.

I think we can approve this for now, at least to go forward, but it would be better to convince @kylef to include that logic in Stencil proper in a later time for consistency between all boolean filters.

@@ -15,6 +15,11 @@ public extension Extension {

// MARK: - Private

private func registerBooleanFilterWithArguments(_ name: String, filter: @escaping Filters.BooleanWithArguments) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just nitpicking, but I wonder if we shouldn't just call this method registerFilter like all the others — and let the type overloading do the magic of using the proper version of registerFilter depending on the kind of filter passed…

Copy link
Contributor

@krzysztofzablocki krzysztofzablocki Nov 15, 2017

Choose a reason for hiding this comment

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

@Antondomashnev up to you, but I think its good idea to leverage inference here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AliSoftware @krzysztofzablocki yes, it's a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, I've tried it but I forgot that call original registerFilter inside the registerBooleanFilterWithArguments and it leads to the recursion 😞 @krzysztofzablocki @AliSoftware do you have any idea how to workaround it?

Copy link
Contributor

@AliSoftware AliSoftware Nov 16, 2017

Choose a reason for hiding this comment

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

Using as to specify the overload you want to call should do it I think

typealias AnyFilter = (Any?, [Any?]) throws -> Any?
registerFilter(name, filter: filter as AnyFilter)
registerFilter("!\(name)", filter: { value, arguments in try !filter(value, arguments) } as AnyFilter)

@ilyapuchka
Copy link
Collaborator

FYI I created a PR that will add this functionality to Stencil stencilproject/Stencil#160

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.

6 participants