Docs [3 of 4] #19
Docs [3 of 4] #19
Conversation
| // Generated from <%= script_name %> | ||
| // | ||
| // <%= type.name %>.swift | ||
| // Buy |
There was a problem hiding this comment.
Why are you adding Buy here?
| } | ||
| <% end %> | ||
|
|
||
| override open func childObjectType(key: String) -> GraphQL.ChildObjectType { |
There was a problem hiding this comment.
These functions are used by the "Relay" layer of Shopify's ios app, so you can't remove them without breaking it right now.
| @@ -221,9 +221,45 @@ def deserialize_value_code(field_name, expr, type, untyped: true) | |||
| raise NotImplementedError, "Unexpected #{type.kind} argument type" | |||
| end | |||
| end | |||
|
|
|||
|
|
|||
| def generate_input_init(type) | |||
There was a problem hiding this comment.
Why move the template code into a function that builds a string? This seems like a step backwards in readability
| text.gsub("\n", " ") | ||
| end | ||
|
|
||
| def input_field_description(type) |
There was a problem hiding this comment.
The reason it's done here is to avoid duplicating the logic across the whole .erb file.
| "@available(*, deprecated#{message_argument})\n" | ||
| end | ||
|
|
||
| def swift_doc(element, include_args=true) |
| def swift_arg_defs(field) | ||
| defs = ["aliasSuffix: String? = nil"] | ||
| defs = ["alias: String? = nil"] |
There was a problem hiding this comment.
These "minor renames" are actually breaking changes. Also, the reason why it isn't just called alias is because we prefix what is passed in with the field name so that there aren't conflicts between fields. By just calling it alias it makes it sound like it is used literally as the GraphQL alias, which makes it seem like the caller needs to worry about preventing conflicts with other fields of the same name.
| end | ||
|
|
||
| def swift_attributes(deprecatable) | ||
| return unless deprecatable.deprecated? | ||
| if deprecatable.deprecation_reason | ||
| message_argument = ", message:#{deprecatable.deprecation_reason.inspect}" | ||
| end | ||
| "@available(*, deprecated#{message_argument})" | ||
| "@available(*, deprecated#{message_argument})\n" |
There was a problem hiding this comment.
why the addition of the \n? existing code seems to generate without this being necessary
| // Generated from <%= script_name %> | ||
| // | ||
| // <%= schema_name %>.swift | ||
| // Buy |
There was a problem hiding this comment.
not always the BUY SDK and idea was brought up in #14 (comment) for a solution
| @@ -9,20 +34,19 @@ import Foundation | |||
| <% end %> | |||
|
|
|||
| <% if type.interface? || type.union? %> | |||
| <%= %> | |||
| <% fields.each do |field| %> | ||
| <%= swift_attributes(field) %> | ||
| var <%= escape_reserved_word(field.camelize_name) %>: <%= swift_output_type(field.type) %> { get } | ||
| <% end %> | ||
| func childResponseObjectMap() -> [GraphQL.AbstractResponse] |
| @@ -192,6 +192,10 @@ public class GraphQL { | |||
| } | |||
| return key | |||
| } | |||
|
|
|||
| internal func childResponseObjectMap() -> [GraphQL.AbstractResponse] { | |||
There was a problem hiding this comment.
why are we introducing an abstract method that used to be defined at the protocol layer? why not leave as part of the protocol instead of having this one with the fatalError?
formatting nitpick: spaces instead of tabs here
| text << "\n" | ||
| end | ||
| text << "}" | ||
| end |
There was a problem hiding this comment.
It looks like you just converted the code from the ERB file into a ruby method with string concatenation. I find the code more readable the way it was before. I also find PRs with miscellaneous changes like this a lot harder to review, since a refactor can hide functional changes.
| doc << "\n" | ||
| doc << '/// - ' + arg.name + ': ' + (arg.description.nil? ? "No description" : format_arg_list(arg.description, 7)) | ||
| end | ||
| doc << "\n///" |
There was a problem hiding this comment.
Please follow the existing coding style. Specifically, here it looks like you are indenting with tabs instead of two spaces per indent
|
Also, CI is 🔴 |
What this does