Fixed using spaces in filter expressions and variables lists #178
Conversation
ilyapuchka
changed the title
ilyapuchka
force-pushed
the
fix-filter-splaces
branch
2 times, most recently
from
140f880 to
d1507d3
Compare
Sources/Tokenizer.swift
Outdated
| } | ||
| } | ||
|
|
||
| // joins back components around characters used in variables lists and filters | ||
| private func smartJoin(_ components: [String]) -> [String] { |
There was a problem hiding this comment.
Does this have performance implications?
There was a problem hiding this comment.
@yonaskolb thanks for pointing it out =) I've reimplemented it, should be faster now.
ilyapuchka
force-pushed
the
fix-filter-splaces
branch
6 times, most recently
from
8ffc3a1 to
2fcb427
Compare
|
@kylef can we merge this? I believe this will improve templates readability |
kylef
force-pushed
the
fix-filter-splaces
branch
from
0218a91 to
118c3f7
Compare
Sources/Tokenizer.swift
Outdated
| let specialCharacters = ",|:" | ||
| func appendWord(_ word: String) { | ||
| if components.count > 0 && | ||
| (specialCharacters.characters.contains(components.last!.characters.last!) || |
There was a problem hiding this comment.
😱 all those force unwraps 😢
There was a problem hiding this comment.
🤷♂️ it's all safe 🙄
There was a problem hiding this comment.
if let lastCompChar = components.last?.characters.last, (specialCharacters.contains(lastCompChar) || …)
There was a problem hiding this comment.
ok, ok, pony saved 🦄 😄
There was a problem hiding this comment.
@AliSoftware done
Sources/Tokenizer.swift
Outdated
| } else { | ||
| components.append(word) | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn't it be more logical to use a Set<Character> here which would seem more appropriate and already handle all that append-without-duplicates logic for you already?
There was a problem hiding this comment.
That's not exactly what we are doing here. We are checking if a word is a special character (for the case when it is surrounded by spaces, i.e. in |), or if previous word ends with a special character, in which case we concatenate last and new word, so ars| default or ars | default(but not ars |default) ends up as ars|default.
| $0.it("can render a filter") { | ||
| let templateString = "{% for article in ars|default:articles %}" + | ||
| $0.it("can render a filter with spaces") { | ||
| let templateString = "{% for article in ars | default: a, b , articles %}" + |
There was a problem hiding this comment.
Not sure I'm familiar with that default filter accepting 3 arguments?
There was a problem hiding this comment.
It supports variadic fallbacks for the default value. If a and b are not defined then articles would be there.
CHANGELOG.md
Outdated
| @@ -24,6 +24,7 @@ | |||
| - Integer literals now resolve into Int values, not Float | |||
| - Fixed accessing properties of optional properties via reflection | |||
| - No longer render optional values in arrays as `Optional(..)` | |||
| - Adds support for using spaces in filter expression | |||
There was a problem hiding this comment.
I think this is an enhancement not a bug fix, it was never a bug it just wasn't designed to work like that before.
There was a problem hiding this comment.
I only considered is as a bug as docs for for tag actually use spaces, but I move it to new features section.
|
@AliSoftware @yonaskolb is it good to go? |
|
I'm still not a fan of force-unwrap even especially when we have better constructs to use instead |
When parsing filter expression spaces were not trimmed in filter name and parameters. Also this fixes using spaces in any other nodes where smart split by spaces is used, i.e. in for node. It allows use of
a, bora , b, but nota ,b. The same about|and:separators. So this also fixes #169