Skip to content
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

Nullability support in Plank #64

Merged
merged 1 commit into from
Aug 17, 2017
Merged

Nullability support in Plank #64

merged 1 commit into from
Aug 17, 2017

Conversation

rahul-malik
Copy link
Collaborator

  • Add nullability information into Schema IR
  • Render 'nonnull' or 'nullable' if a property is present in 'required'
    property.
  • Update Cocoa examples to incorporate nullability cases
  • Improve unit test coverage in ObjCIR.swift
  • Add documentation for nullability support

@rahul-malik rahul-malik requested review from maicki and bkase July 24, 2017 18:20
@ghost
Copy link

ghost commented Jul 24, 2017

🚫 CI failed with log

@maicki
Copy link
Contributor

maicki commented Jul 25, 2017

@rahul-malik Here is a diff for adding nullability support to JS / Flow as well as fixes the lint error.

JSFlowNullable.diff.zip

That's a diff to fix the error that should work in Swift 3 and 4:

FixStringLengthError.diff.zip

After applying both, CI should pass.

- Add nullability information into Schema IR
- Render 'nonnull' or 'nullable' if a property is present in 'required'
  property.
- Update Cocoa / FlowJS examples to incorporate nullability cases
- Improve unit test coverage in ObjCIR.swift
- Add documentation for nullability support
@rahul-malik
Copy link
Collaborator Author

@maicki - Please take a look when you get a chance, I've merged your patches and ensured the build compiles and tests pass locally.

@ghost
Copy link

ghost commented Aug 16, 2017

2 Warnings
⚠️ This is a big PR, please consider splitting it up to ease code review.
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

Looks good to me.

case nonnull
}

// Ask @bkase about whether this makes more sense to wrap schemas to express nullability
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @bkase

@@ -242,14 +255,18 @@ extension Schema {
}))
}
case JSONType.object:
let requiredProps = Set(propertyInfo["required"] as? [String] ?? [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but as an idea for all of this stringly accessed property infos should we create an enum for that includes the available property info keys?

@rahul-malik rahul-malik merged commit 864abac into master Aug 17, 2017
@rahul-malik rahul-malik deleted the rmalik-nullability branch August 18, 2017 00:54
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