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 method to register boolean filters #160

Merged
merged 9 commits into from
Sep 25, 2018
Merged

Conversation

ilyapuchka
Copy link
Collaborator

This was touched on in the discussion here SwiftGen/StencilSwiftKit#69 and we are extensively using such approach in Sourcery for our custom filters. Would be nice if it would be available out of the box.

Benefit of such approach compared to using not, as mentioned in the discussion, is that it works with filters chaining.

@ilyapuchka
Copy link
Collaborator Author

@AliSoftware @djbe this is what is there already in StencilSwiftKit, so consider it first step in moving filters from StencilSwiftKit

.render(Context(dictionary: ["value": 1], environment: Environment(extensions: [repeatExtension])))
try expect(result) == "true"

let negativeResult = try Template(templateString: "{{ value|!isPositive }}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think isNegative would be far clearer name for the filter. I'd suggest that the API allows defining the boolean name instead of !{name} to promote clearer Stencil template filters.

Copy link
Collaborator Author

@ilyapuchka ilyapuchka May 12, 2018

Choose a reason for hiding this comment

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

This is just test example, of course isNegative makes more sense. What do you mean by API allows defining the boolean name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The registerBooleanFilter API could allow supplying an opposite filter name. So for example if you wanted to make a boolean isPositive and isNegative this API would allow you to do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, it will make it more generic and will still allow usage ! for those tools which want it (i.e. Sourcery) 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kylef done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, just thought if it was built in they wouldn't have to do that and decide which name and syntax they should use (!, not, negative etc). We could standardise it for them (in this case using !)

/// negativeFilterName defaults to !name
public func registerBooleanFilter(name: String, negativeFilterName: String? = nil, filter: @escaping (Any?) throws -> Bool?) {
    filters[name] = .simple(filter)
    filters[negativeFilterName ?? "!\(name)"] = .simple {
      guard let result = try filter($0) else { return nil }
      return !result
    }
   }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kylef what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@djbe @AliSoftware any feedback on that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think whenever we declare a boolean filter the !name should always exist. Providing another name for the negation should be possible but optional.

This means that if you register isPositive then !isPositive should always exist. You could optionally provide an alternate name for the opposite filter e.g. isNegative and in that case isPositive, isNegative, !isPositive, !isNegative should all exist

I'd go in that direction for two reasons:

  1. Templates writers shouldn't bother that those ! are actually part of the filter names. When they write !isNegative they think that ! is an operator and isNegative is the filter name. So they'd expect ! to work on all boolean filters, not just the ones that don't have a custom inverse
  • When in compound expressions, especially now that we'll support parentheses in expressions with recent PR, some might find more clear in certain context to explicitly use ! isPositive rather than isNegative

Last but not least, isPositive shouldn't be the opposite of isNegative anyway. If we have two filters with those names in Stencil, I for one would expect isPositive(0) and isNegative(0) to both return the same. So I'd argue that this filter isn't even a good candidate for a custom name (at least not this one)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AliSoftware if one registers filter in extension using this new method it will add a counterpart filter, so it's not up to template writer, but the author of extension to choose how filter is registered and how counterpart is named. I don't think Stencil should enforce using ! in this context unless it becomes a "first-class" in boolean expression (and supplement not operator). Some tools may prefer using not{filter} instead and this API will allow them to do that.

@ilyapuchka
Copy link
Collaborator Author

@kylef @yonaskolb @djbe can we move on with this please?

Copy link
Contributor

@djbe djbe left a comment

Choose a reason for hiding this comment

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

This almost LGTM to me, only the docs are missing (see http://stencil.fuller.li/en/latest/custom-template-tags-and-filters.html).

That and a rebase on master of course.

@ilyapuchka
Copy link
Collaborator Author

@djbe updated docs, also removed Boolean from method name as it is unambiguous anyway

Copy link
Contributor

@djbe djbe left a comment

Choose a reason for hiding this comment

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

Great! And good idea with the register function name, I hadn't thought of that!

@ilyapuchka ilyapuchka merged commit fce3dc5 into master Sep 25, 2018
@ilyapuchka ilyapuchka deleted the boolean-filters branch September 25, 2018 22:29
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.

5 participants