Skip to content

Support atoms with multiple types #73

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 9, 2019
Merged

Support atoms with multiple types #73

merged 2 commits into from
Apr 9, 2019

Conversation

jennybuckley
Copy link

@jennybuckley jennybuckley commented Mar 27, 2019 •
edited
Loading

Get rid of schema.DeduceType and instead allow atoms with multiple types defined and use the type of data to eliminate incompatible types.

Also changes Untyped Deduced to this:

types:
- name: __untyped_deduced_
  scalar: untyped
  list:
    elementType:
      namedType: __untyped_atomic_
    elementRelationship: atomic
  map:
    elementType:
      namedType: __untyped_deduced_
    elementRelationship: separable

This is necessary if we want to support things like JSONPropsOrArray in the future

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 27, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jennybuckley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 27, 2019
jennybuckley
jennybuckley commented Mar 27, 2019
View reviewed changes
typed/merge.go
errs = append(errs, w.derefMapOrStruct("lhs: ", "map", w.lhs, &lhs)...)
errs = append(errs, w.derefMapOrStruct("rhs: ", "map", w.rhs, &rhs)...)
w.derefMapOrStruct("lhs: ", "map", w.lhs, &lhs)
w.derefMapOrStruct("rhs: ", "map", w.rhs, &rhs)
Copy link
Author

@jennybuckley jennybuckley Mar 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We could actually remove these errors from all the cases (because the type errors are caught earlier now), but I wanted to keep the diff smaller

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't think that they should ever happen, can we panic?

Copy link
Author

Choose a reason for hiding this comment

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

the error case can happen, but what we want it to do in that case is treat the value as nil

@jennybuckley jennybuckley changed the title Support atoms with multiple types [WIP] Support atoms with multiple types Mar 28, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2019
@jennybuckley jennybuckley force-pushed the anyof-atom branch 2 times, most recently from ccb0e03 to 1176386 Compare April 2, 2019 21:18
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 2, 2019
apelisse
apelisse reviewed Apr 3, 2019
View reviewed changes
Copy link
Contributor

@apelisse apelisse left a comment

Choose a reason for hiding this comment

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

There is not much to say, this very cool again

typed/merge.go Outdated
alhs := deduceAtom(a, w.lhs)
arhs := deduceAtom(a, w.rhs)
if reflect.DeepEqual(alhs, arhs) {
errs =append(errs, handleAtom(arhs, w.typeRef, w)...)
Copy link
Contributor

Choose a reason for hiding this comment

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

very important: missing space :-)

@apelisse
Copy link
Contributor

apelisse commented Apr 4, 2019

Do you want to squash and I lgtm or do you want to move the type to schemaconv?

@apelisse
Copy link
Contributor

apelisse commented Apr 7, 2019

I think we agree that it was better before that last commit?

@apelisse
Copy link
Contributor

apelisse commented Apr 9, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2019
@apelisse apelisse changed the title [WIP] Support atoms with multiple types Support atoms with multiple types Apr 9, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 9, 2019
@apelisse
Copy link
Contributor

apelisse commented Apr 9, 2019

We need to add a little bit more testing for this feature, let's follow-up after.
Also we need to fix the gofmt thing, I'll probably force merge.

@apelisse apelisse merged commit a0d2dce into kubernetes-sigs:master Apr 9, 2019
@jennybuckley jennybuckley mentioned this pull request Dec 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apelisse apelisse apelisse left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

Assignees

@apelisse apelisse

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jennybuckley @k8s-ci-robot @apelisse