-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GSOC-Week22 #2
base: main
Are you sure you want to change the base?
GSOC-Week22 #2
Conversation
- add cases for testSAMJSONSchemaReader() unitTest to pass AWSLambdaDeploymentDescriptorGeneratorTest tests
- removed ArrayItem struct and updated JSONType to adapt to the removal of the struct - added DecodingError to catch error when decoding fails - handled smart decoding of type key inside the JSONType - added more test cases for the SimpleJSONSchemaReader as a refrence to be able to add them later for the JSONSchemaReader - added more keys for the ObjectSchema struct - removed keys from SAMJSONSchema to pass tests on minimal keys - handled one scencario of the anyOf key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks quite good - we're almost there. I added a few comments and suggestions for changes
@@ -1,21 +1,69 @@ | |||
import Foundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you don't use regular expressions anymore, I think this can be remove.
import Foundation | ||
|
||
//MARK: - JSONSchema .. | ||
|
||
struct JSONSchema: Decodable { | ||
let id: String? | ||
let schema: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that you have a type to represent this, you can write
let schema : SchemaVersion
init(from decoder: Decoder) throws { | ||
let container = try decoder.container(keyedBy: CodingKeys.self) | ||
self.id = try container.decodeIfPresent(String.self, forKey: .id) | ||
self.schema = try container.decode(String.self, forKey: .schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to decode as a SchemaVersion
type instead of a string
This will require to add an init(from:)
method on SchemaVersion
Hint : this init(from:)
will use a singleValue
Container
public init(from decoder: any Decoder) throws {
let container = try decoder.singleValueContainer()
...
let container = try decoder.container(keyedBy: CodingKeys.self) | ||
self.id = try container.decodeIfPresent(String.self, forKey: .id) | ||
self.schema = try container.decode(String.self, forKey: .schema) | ||
guard schema == SchemaVersion.draft4.rawValue else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the change of type is done, you can further simplify this test
guard schema == .draft4 else {
...
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deserves a few comments to explain the dance you're doing here, doesn't it ?
let container = try decoder.singleValueContainer() | ||
let jsonType = try container.decode(JSONType.self) | ||
self = .type(jsonType) | ||
do { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the do
can be removed
} | ||
|
||
return jsonType | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the above function, maybe we can add helper functions to also extract the all
and any
values.
Something like
public func any() -> [JSONType]? {
guard case .anyOf(let anyOf) = self else {
fatalError("not an anyOf")
}
return anyOf
}
public func all() -> [JSONUnionType]? {
guard case .allOf(let allOf) = self else {
fatalError("not an allOf")
}
return allOf
}
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too many empty lines :-)
} else { | ||
self.subSchema = nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too many empty lines here.
Maybe you can you swiftformat
command to reformat the code (there is a configuration file .swiftformat
in the project directory)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSONSchemaReader looks good to me ! Great !
Modifications on JSONSchemaReader based on example provided