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

fix(dotnet): fix deep type conversion across the process boundary, intelisense docs, set target to netcoreapp2.1 #772

Merged
merged 12 commits into from
Sep 18, 2019
Merged

fix(dotnet): fix deep type conversion across the process boundary, intelisense docs, set target to netcoreapp2.1 #772

merged 12 commits into from
Sep 18, 2019

Conversation

assyadh
Copy link
Contributor

@assyadh assyadh commented Sep 11, 2019 •
edited
Loading

This PR fixes various issues due to how we convert collection elements, and how the type reference is passed when converting.

These two issues are similar and are due to nested Arrays of object not being casted properly:
Fixes aws/aws-cdk#3244
Fixes aws/aws-cdk#3672
Added a unit test to cover the case

This issue is due to Maps of Arrays of Anys not being casted properly:
Fixes aws/aws-cdk#3813
Added a unit test to cover the case

Also added support for xml documentation which is now added to the NuGet packages and should allow intellisense to provide in IDE help:
Fixes #749
Fixes aws/aws-cdk#1846

Migrated the target framework to netcoreapp2.1 instead of netstandard2.0:
Fixes #714

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@assyadh assyadh requested a review from a team as a code owner September 11, 2019 21:49
@mergify
Copy link
Contributor

mergify bot commented Sep 11, 2019

The title of this Pull Request does not conform with [Conventional Commits] guidelines. It will need to be adjusted before the PR can be merged.
[Conventional Commits]: https://www.conventionalcommits.org

@assyadh assyadh changed the title wip(dotnet): Fix #3244 and all the arrays of objects issues wip(dotnet): Fix #3244 and all the arrays of objects issues + various milestones for GA Sep 11, 2019
@assyadh assyadh mentioned this pull request Sep 11, 2019
Closed
@eladb
Copy link
Contributor

eladb commented Sep 12, 2019

Please update PR description and title so this will be reviewable

@eladb
Copy link
Contributor

eladb commented Sep 12, 2019

The description for "fix" should describe the root cause

@assyadh assyadh changed the title wip(dotnet): Fix #3244 and all the arrays of objects issues + various milestones for GA fix(dotnet): Fix #3244 and all the arrays of objects issues + various milestones for GA Sep 12, 2019
@assyadh assyadh changed the title fix(dotnet): Fix #3244 and all the arrays of objects issues + various milestones for GA fix(dotnet): Fix the "Could not infer JSII type for .NET type" exceptions + intelisense docs and netcoreapp2.1 target Sep 12, 2019
@RomainMuller RomainMuller changed the title fix(dotnet): Fix the "Could not infer JSII type for .NET type" exceptions + intelisense docs and netcoreapp2.1 target fix(dotnet): fix deep type conversion across the process boundary, intelisense docs, set target to netcoreapp2.1 Sep 16, 2019
RomainMuller
RomainMuller previously approved these changes Sep 16, 2019
View reviewed changes
@mergify mergify bot dismissed RomainMuller’s stale review September 16, 2019 09:59

Pull request has been modified.

RomainMuller
RomainMuller previously approved these changes Sep 16, 2019
View reviewed changes
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@RomainMuller RomainMuller requested review from costleya and removed request for costleya September 17, 2019 16:17
@mergify mergify bot dismissed RomainMuller’s stale review September 17, 2019 18:35

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

costleya
costleya previously approved these changes Sep 17, 2019
View reviewed changes
.../jsii-dotnet-runtime/src/Amazon.JSII.Runtime/Services/Converters/FrameworkToJsiiConverter.cs Outdated
private bool TryConvertCollectionElement(object element, IReferenceMap referenceMap, TypeReference elementType,
out object convertedElement)
{
bool converted;
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we just return directly instead of having temporary variables and a break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mergify mergify bot dismissed costleya’s stale review September 17, 2019 18:51

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@assyadh
Copy link
Contributor Author

assyadh commented Sep 17, 2019 •
edited
Loading

This can be squashed and merged, double approved.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@assyadh assyadh mentioned this pull request Sep 17, 2019
Closed
@RomainMuller RomainMuller merged commit ecf8d3b into master Sep 18, 2019
@RomainMuller RomainMuller deleted the hamzaad/dotnet-misc branch September 18, 2019 06:51
@costleya costleya mentioned this pull request Sep 25, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomainMuller RomainMuller RomainMuller approved these changes

@costleya costleya costleya left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@assyadh @eladb @aws-cdk-automation @RomainMuller @costleya