Added indent filter #188
Added indent filter #188
Conversation
There was a problem hiding this comment.
Looks good, I have a few suggestions. Could you also please add the ident filter to the documentation https://github.com/kylef/Stencil/blob/master/docs/builtins.rst#built-in-filters with an example and description.
Sources/Filters.swift
Outdated
| @@ -26,7 +26,7 @@ func defaultFilter(value: Any?, arguments: [Any?]) -> Any? { | |||
| } | |||
|
|
|||
| func joinFilter(value: Any?, arguments: [Any?]) throws -> Any? { | |||
| guard arguments.count < 2 else { | |||
| guard arguments.count <= 1 else { | |||
There was a problem hiding this comment.
Can you break this into separate PR and include tests for this change please?
There was a problem hiding this comment.
This does not change the logic, I just prefer <= over <. I'll revert to not have unneeded changes
Sources/Filters.swift
Outdated
|
|
||
| func indentFilter(value: Any?, arguments: [Any?]) throws -> Any? { | ||
| guard !arguments.isEmpty else { | ||
| throw TemplateSyntaxError("'indent' filter takes at least 1 argument") |
There was a problem hiding this comment.
I'd perhaps make this work without any arguments. Defaulting the width to 4 spaces.
{{ names|indent }}
Sources/Filters.swift
Outdated
| } | ||
|
|
||
| guard let indentWidth = arguments[0] as? Int else { | ||
| throw TemplateSyntaxError("first argument should be Int") |
There was a problem hiding this comment.
I think this error message could be clearer as the following, including filter name along with the argument label and what the user mistakenly put.
'indent' filter width must be an integer ('{{ the width user provided }}')
Similar for errors below.
Tests/StencilTests/FilterSpec.swift
Outdated
| let result = try template.render(Context(dictionary: ["value": "One\nTwo"])) | ||
| try expect(result) == " One\n Two" | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you add a test where the value has a trailing new line. I think the behaviour is currently that the trailing new line will be removed due to the split/join. Would be great to confirm and fix if so.
There was a problem hiding this comment.
This should be fine as lines are then joined with new line separator and when they are split empty lines will be preserved. Added that to the test for empty lines.
Tests/StencilTests/FilterSpec.swift
Outdated
| let result = try template.render(Context(dictionary: ["value": "One\nTwo"])) | ||
| try expect(result) == " One\n Two" | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you please add a test for a value that has empty lines? For example: Line 1\n\n\nLine 3\n. The condition for empty lines is not tested:
return result + [(line.isEmpty ? "" : "\(indentation)\(line)")]
|
@kylef all your comments addressed, thanks for taking time to review this =) |
|
I think one was missed (documentation): #188 (review), otherwise good to go. |
|
Ah, yes, didn't notice that. Documentation should be rebuilt for other PR's too, so I was thinking to do it later with one PR, but can do it now too. |
|
I'm fine with that too, so will merge when CI is finished and you can make a separate PR. |
|
\cc @djbe we should now use that instead of our |
|
@kylef this is ready to be merged |
With this filter it is possible to indent content either of variable or included from another template.
Examples:
will render:
With
filtertag indentation can be also applied to included template content:will render:
Resolves #95