Parse string function #65
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
djbe
requested changes
Aug 29, 2017
CHANGELOG.md
Outdated
| _None_ | ||
| * Accept `LosslessStringConvertible` input for strings filters. | ||
| [Antondomashnev](https://github.com/antondomashnev) | ||
| [#59](https://github.com/SwiftGen/StencilSwiftKit/pull/63) |
| /// - arguments: an array of argument values, may be empty | ||
| /// - index: the index in the arguments array | ||
| /// - Throws: Filters.Error.invalidInputType | ||
| static func parseStringArgument(from arguments: [Any?], at index: Int = 0) throws -> String { |
There was a problem hiding this comment.
To keep the naming consistent with the other filters, we could consider making the index mandatory (no default value). And modify the other parse argument functions (bool/enum) to require an index as well.
There was a problem hiding this comment.
Honestly I don't see the benefit for this, we'll increase the number of repeatable code in order to have naming consistency. I think it doesn't worth it 😄
|
@djbe @AliSoftware ping 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves #64, but with one tweak, I've realised that
CustomStringConvertibleis too broad protocol, that has many adoptions which not really a fit to be a filter parameter, so I usedLosslessStringConvertible. I think it's a good tradeoff between making input not that strict, but still with some sense.Regarding the functions I've tried to follow the pattern in
Filters.swiftbut I had to introduce two functions - parse array ofAny?and justAny?as it reduced the possible boilerplate code. I've also tried to use the same name for these two functions:but it lead to confusion for compiler as
Any?could be anything, also an array ofAny?😄I thought maybe we can have an
Extractorinstead of having these functions inFilters, what do you think?