Print nested types in errors, hash keys using full typenames #221
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
ilyapuchka
reviewed
Apr 2, 2019
| } | ||
|
|
||
| func reflect(_ type: Any.Type) -> String | ||
| { | ||
| return String(reflecting: type) |
There was a problem hiding this comment.
can you please just use this rather than introducing free function?
ilyapuchka
reviewed
Nov 10, 2019
| @@ -41,7 +41,7 @@ public struct DefinitionKey: Hashable, CustomStringConvertible { | |||
| } | |||
|
|
|||
| public var description: String { | |||
| return "type: \(type), arguments: \(typeOfArguments), tag: \(tag.desc)" | |||
| return "type: \(reflect(type)), arguments: \(typeOfArguments), tag: \(tag.desc)" | |||
There was a problem hiding this comment.
Suggested change
| return "type: \(reflect(type)), arguments: \(typeOfArguments), tag: \(tag.desc)" | |
| return "type: \(String(reflecting: type)), arguments: \(typeOfArguments), tag: \(tag.desc)" |
ilyapuchka
reviewed
Nov 10, 2019
| @@ -77,18 +77,21 @@ public enum DipError: Error, CustomStringConvertible { | |||
| public var description: String { | |||
| switch self { | |||
| case let .definitionNotFound(key): | |||
| return "No definition registered for \(key).\nCheck the tag, type you try to resolve, number, order and types of runtime arguments passed to `resolve()` and match them with registered factories for type \(key.type)." | |||
| return "No definition registered for \(key).\nCheck the tag, type you try to resolve, number, order and types of runtime arguments passed to `resolve()` and match them with registered factories for type \(reflect(key.type))." | |||
There was a problem hiding this comment.
Suggested change
| return "No definition registered for \(key).\nCheck the tag, type you try to resolve, number, order and types of runtime arguments passed to `resolve()` and match them with registered factories for type \(reflect(key.type))." | |
| return "No definition registered for \(key).\nCheck the tag, type you try to resolve, number, order and types of runtime arguments passed to `resolve()` and match them with registered factories for type \(String(reflecting: key.type))." |
ilyapuchka
reviewed
Nov 10, 2019
| case let .autoInjectionFailed(label, type, error): | ||
| return "Failed to auto-inject property \"\(label.desc)\" of type \(type). \(error)" | ||
| return "Failed to auto-inject property \"\(label.desc)\" of type \(reflect(type)). \(error)" |
There was a problem hiding this comment.
Suggested change
| return "Failed to auto-inject property \"\(label.desc)\" of type \(reflect(type)). \(error)" | |
| return "Failed to auto-inject property \"\(label.desc)\" of type \(String(reflecting: type)). \(error)" |
ilyapuchka
reviewed
Nov 10, 2019
| case let .autoWiringFailed(type, error): | ||
| return "Failed to auto-wire type \"\(type)\". \(error)" | ||
| return "Failed to auto-wire type \"\(reflect(type))\". \(error)" |
There was a problem hiding this comment.
Suggested change
| return "Failed to auto-wire type \"\(reflect(type))\". \(error)" | |
| return "Failed to auto-wire type \"\(String(reflecting: type))\". \(error)" |
ilyapuchka
reviewed
Nov 10, 2019
| case let .ambiguousDefinitions(type, definitions): | ||
| return "Ambiguous definitions for \(type):\n" + | ||
| return "Ambiguous definitions for \(reflect(type)):\n" + |
There was a problem hiding this comment.
Suggested change
| return "Ambiguous definitions for \(reflect(type)):\n" + | |
| return "Ambiguous definitions for \(String(reflecting: type)):\n" + |
ilyapuchka
reviewed
Nov 10, 2019
| definitions.map({ "\($0)" }).joined(separator: ";\n") | ||
| case let .invalidType(resolved, key): | ||
| return "Resolved instance \(resolved ?? "nil") does not implement expected type \(key.type)." | ||
| return "Resolved instance \(resolved ?? "nil") does not implement expected type \(reflect(key.type))." |
There was a problem hiding this comment.
Suggested change
| return "Resolved instance \(resolved ?? "nil") does not implement expected type \(reflect(key.type))." | |
| return "Resolved instance \(resolved ?? "nil") does not implement expected type \(String(reflecting: key.type))." |
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.
Hi Ilya, we're using nested types a lot and the "leafs" have identical names, which leads to conflicts when type matching and non-specific log messages. This change uses the fully scoped names.