Skip to content

InputValue [4 of 4] #14

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

Closed
wants to merge 2 commits into from
Closed

InputValue [4 of 4] #14

wants to merge 2 commits into from

Conversation

dbart01
Copy link
Contributor

@dbart01 dbart01 commented Sep 8, 2017

What this does

  • Introduces explicit support for null values via InputType<T>
  • Deprecates applicable initializers for input objects in generated code

Relies on #19

- add InputValue type and extensions
- modify generation to use InputValue enums in serialize()
- declare all optional values in Input objects at InputValue<T>
@dbart01 dbart01 requested a review from BenEmdon September 8, 2017 18:54
@dbart01 dbart01 changed the base branch from master to task/revert-explicit-null September 8, 2017 18:55
Copy link
Contributor

@BenEmdon BenEmdon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am starting to feel that the PR should be split up slightly. The explicit nulls are controversial enough to be their own PR. Also we shouldn't try to template swift code outside an erb, that feels wrong.

input_fields.each do |field|
text << escape_reserved_word(field.camelize_name)
text << ": "
text << swift_input_type(field.type, wrapped: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a painful way to generate code 😕 I think graphql_swift_gen.rb is more of a place to do some calculated logic rather then template the entire init. Having a function that is constantly appening to a string isn't nearly as readable or friendly as using embedded ruby. This should be in type.swift.erb.

// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding this license to all generated files why not pass in any licence you want based on the use case? Like if this is part of Buy SDK the pass in the buy SDK license.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea 👍


public var orUndefined: InputValue<Wrapped> {
return InputValue(orUndefined: self)
}
Copy link
Contributor

@BenEmdon BenEmdon Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These optional extensions feel redundant to me. Not everyone will want to use them. These could be removed from the generator. Consumers that want this extension can always build it themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Agreed.

@dbart01 dbart01 changed the base branch from task/revert-explicit-null to task/docs September 11, 2017 20:27
@dbart01 dbart01 changed the title Buy SDK InputValue [4 of 4] Sep 11, 2017
@dbart01 dbart01 requested a review from g-Off September 11, 2017 20:29
@g-Off g-Off mentioned this pull request Sep 11, 2017
@dbart01 dbart01 closed this Sep 12, 2017
@dbart01 dbart01 deleted the buy-sdk branch May 10, 2018 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants