Parsing tuples #92
Parsing tuples #92
Conversation
Generated by 🚫 danger |
| } | ||
|
|
||
| // sourcery: skipDescription | ||
| final class TypeName: NSObject, AutoDiffable { | ||
| let name: String | ||
|
|
||
| /// Actual type name if given type name is type alias |
There was a problem hiding this comment.
so how is this going to happen now? I think that before this PR TypeName was never used for typealiases
There was a problem hiding this comment.
Before we had only typeName and type, and type will be set as actual type even when type is an alias, but only if it is an alias for known type. It will not work in case of types that are not Type at all, like closures and tuples. So when resolving type we now not just set the type property, but also the name of the actual type. Nothing else changes.
I think it makes sense to allow access to both type names so user can decide what to use - what is used in code, or what will be used in runtime.
Also in future we should improve how we handle typealiases for optional types, i.e. typealias Handler = (()->())?. Not saying that this is a "good" practice, but it's just valid Swift, so maybe we should support that too, or just agree to ignore that case for now.
|
|
||
| lazy private(set) var elements: [Element] = { | ||
| let trimmedBracketsName = String(self.name.characters.dropFirst().dropLast()) | ||
| return trimmedBracketsName |
There was a problem hiding this comment.
I really think Parsing should happen in the FileParser or one of associated files rather than model objects
There was a problem hiding this comment.
Yeah, I will try to move that to Parser. Actually I even tried to make Enum.Case.AssociatedValue an alias for TupleType.Element as in fact they are tuples, but have to workaround the fact that tuple of single item is not considered a tuple in this pr.
| .enumerated() | ||
| .map { | ||
| let nameAndType = $1.colonSeparated().map({ $0.trimmingCharacters(in: .whitespaces) }) | ||
| let defaultName: String? = $0 == 0 && items.count == 1 ? nil : "\($0)" |
There was a problem hiding this comment.
why not use switch? and can you explain what is that ternary logic suppose to do here?
There was a problem hiding this comment.
In case of single associated value it will not have a name. Even though we can set it, but we can not access it when we bind it. In this case the name will be nil, otherwise if there is no name, or it is _ we use index.
Btw I think we should change it, because though we can not access single associated value by name when binding, we must use it when creating an enum:
enum Some { case value(v: Int) }
if case let .value(a) = Some.value(v: 1) { }It looks like it should be handled like method parameters names (argument label + parameter name)
There was a problem hiding this comment.
do you feel comfortable tweaking that in this PR or should we do it later on separately?
There was a problem hiding this comment.
I'll try to do it here, if it will take too much of an effort I let you know, but should be not hard to fix
There was a problem hiding this comment.
@krzysztofzablocki any ideas what names better to use for that? localName (ex name) and externalName?
To better illustrate the issue here are few cases:
enum Some {
case option0(Int)
case option1(v: Int)
case option2(v: Int, Int)
}
let opt1 = Some.option0(1) //no local name
let opt2 = Some.option1(v: 1) //has local name
let opt3 = Some.option2(v: 1, 2) //first has local name, second has no local name
switch some {
case let .option0(a): print(a) // has no external name
case let .option1(a): print(a) // has no external name
case let .option2(a): print(a.first); print(a.1) //first has external name, second has index name
}The same applies for tuples except that single element tuples are not allowed to have name.
There was a problem hiding this comment.
would we have 3 different properties? local external and index ? or would external automatically get index name as we do now?
There was a problem hiding this comment.
yes, it also makes sense to have index/indexName as elements are always accessible by their index
There was a problem hiding this comment.
ok, if external name automatically gets the indexName in case nothing else is available (callsite convenience) this looks reasonable 👍
| let defaultName: String? = $0 == 0 && items.count == 1 ? nil : "\($0)" | ||
|
|
||
| guard nameAndType.count == 2 else { | ||
| return Enum.Case.AssociatedValue(name: defaultName, typeName: $1) |
There was a problem hiding this comment.
what use case is this correct in?
There was a problem hiding this comment.
case value(first: Int, Int), it's the case of the second associated value, nameAndType will have count 1 and well contain just Int. defaultName will be "1" in this case.
There was a problem hiding this comment.
thanks for explaining :)
| func registerFilterWithArguments<A>(_ name: String, filter: @escaping (Any?, A) throws -> Any?) { | ||
| registerFilter(name) { (any, args) throws -> Any? in | ||
| guard args.count == 1, let arg = args.first as? A else { | ||
| throw TemplateSyntaxError("'\(name)' filter takes a single string argument") |
There was a problem hiding this comment.
you should use the A type not string in this message
|
|
||
| ext.registerFilter("computed", filter: Filter<Variable>.make({ $0.isComputed && !$0.isStatic })) | ||
| ext.registerFilter("stored", filter: Filter<Variable>.make({ !$0.isComputed && !$0.isStatic })) | ||
| ext.registerFilter("tuple", filter: Filter<Variable>.make({ $0.isTuple })) | ||
|
|
||
| ext.registerFilterWithArguments("based", filter: FilterOr<Type, Typed>.make({ $0.based[$1] != nil }, other: { $0.type?.based[$1] != nil })) |
|
I'll finish this tonight
Ilya Puchka
Отправлено с iPhone
… 4 янв. 2017 г., в 7:51, Krzysztof Zabłocki ***@***.***> написал(а):
Anything I can help you with in this pr ? I want to get this one and #95 merged before I do PR for the #96 / #97
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
# Conflicts: # CHANGELOG.md # Sourcery/Models/Enum.swift # Sourcery/Models/TypeName.swift
|
ready for review? |
|
Yes |
| @@ -6,9 +6,15 @@ | |||
| ### New Features | |||
| - Variables with default initializer are now supported, e.g. `var variable = Type(...)` | |||
| - Added support for special escaped names in enum cases e.g. `default` or `for` | |||
| - Added support for tuple types and `tuple` filter for variables | |||
| - Enum associated values now have `localName` and `exteranlName` properties. | |||
|
|
||
| - `name` <- element name | ||
| - `typeName` <- element type name (*TypeName*) | ||
| - `type` <- type of element, if known |
There was a problem hiding this comment.
missing the shorthands for optional and unwrapped
| override var description: String { | ||
| return name | ||
| if let actualTypeName = actualTypeName { |
There was a problem hiding this comment.
I don't think this should be doing anything like aka, this isn't debugDescription, the reason I've made it return name is because it's convienent to allow users to render it instead of having to reach through to name. So it should what you'd like to generate on callsite
| return components(separatedBy: ":", excludingDelimiterBetween: ("<[(", ")]>")) | ||
| } | ||
|
|
||
| fileprivate func components(separatedBy delimiter: Character, excludingDelimiterBetween between: (open: String, close: String)) -> [String] { |
There was a problem hiding this comment.
awesome, I think I can use this to solve one bug I noticed :)
|
Fixed your comments |
Resolves #48