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.

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

Introduced non-generic type descriptor method descriptor.Type(System.Type). #1081

Merged
merged 6 commits into from
Sep 19, 2019
Merged

Introduced non-generic type descriptor method descriptor.Type(System.Type). #1081

merged 6 commits into from
Sep 19, 2019

Conversation

promontis
Copy link
Contributor

Addresses #1079

@michaelstaib michaelstaib changed the title Resolved #1079 Introduced non-generic type descriptor method descriptor.Type(System.Type). Sep 17, 2019
@michaelstaib michaelstaib added this to the 10.2.0 milestone Sep 17, 2019
michaelstaib
michaelstaib requested changes Sep 18, 2019
View reviewed changes
Copy link
Member

@michaelstaib michaelstaib left a comment

Choose a reason for hiding this comment

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

I left some comments on the code.

We also need tests for each of these new methods. Moreover we have to bring the methods to the interface level.

I will check if there are more places we should have a look at.

src/Core/Types/Types/Descriptors/ArgumentDescriptorBase~1.cs Show resolved Hide resolved
src/Core/Types/Types/Descriptors/OutputFieldDescriptorBase.cs Show resolved Hide resolved
@promontis
Copy link
Contributor Author

@michaelstaib thanks for having a quick look! I'll add the tests, pull to the interface and resolve you comments! Probably tonight or tomorrow

@promontis
Copy link
Contributor Author

@michaelstaib I've added tests, pulled to interface, resolved the comments and fixed a bug.

Is it ok like this?

Btw, I didn't see any OutputFieldDescriptorTests.cs. Do you want me to add it?

michaelstaib
michaelstaib approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@michaelstaib michaelstaib left a comment

Choose a reason for hiding this comment

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

This PR looks now pretty good. I will trigger a CI.

@promontis
Copy link
Contributor Author

@michaelstaib is it ready to be merged :) ? If so, could you also build a preview nuget?

@michaelstaib
Copy link
Member

@promontis sorry was in a meeting ... yes everything is green :) I will trigger the merge and release.

@michaelstaib michaelstaib merged commit d0e68a5 into ChilliCream:version_10_0_0_master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelstaib michaelstaib michaelstaib approved these changes

Assignees

@promontis promontis

Labels
None yet
Projects
None yet
Milestone
10.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@promontis @michaelstaib