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

fix: process null values #1248

Merged
merged 17 commits into from
Aug 13, 2024
Merged

Conversation

Kindest13
Copy link
Contributor

@Kindest13 Kindest13 commented Jun 26, 2024

Fixes #1188
1 part for processing null values

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

thanks for starting this. I have left a number of comments to move it forward.

packages/abstractions/src/serialization/parseNode.ts Outdated Show resolved Hide resolved
packages/serialization/json/src/jsonParseNode.ts Outdated Show resolved Hide resolved
packages/serialization/json/src/jsonParseNode.ts Outdated Show resolved Hide resolved
@Kindest13 Kindest13 requested a review from baywet June 27, 2024 12:58
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

I think we're making good progress here! Thanks for the continuous efforts.

packages/abstractions/src/serialization/kiotaSerializer.ts Outdated Show resolved Hide resolved
packages/abstractions/src/serialization/parseNode.ts Outdated Show resolved Hide resolved
packages/serialization/form/src/formParseNode.ts Outdated Show resolved Hide resolved
packages/serialization/text/src/textParseNode.ts Outdated Show resolved Hide resolved
packages/serialization/text/src/textSerializationWriter.ts Outdated Show resolved Hide resolved
@Kindest13
Copy link
Contributor Author

LGTM! @andrueastman for a final review

Great! I just comitted changes to remove .only from test file

baywet
baywet previously approved these changes Jul 11, 2024
andrueastman
andrueastman previously approved these changes Jul 12, 2024
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

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

LGTM.

@Kindest13 I believe you still need to reply to the policy service bot to get this merged.

@Kindest13
Copy link
Contributor Author

@Kindest13 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@baywet
Copy link
Member

baywet commented Jul 12, 2024

Now this PR is ready, I'd like us to get started on the generation updates before we merge this pull request.
@Kindest13 do you have all the information you need to get started in the kiota repository or do you want additional pointers?

@Kindest13
Copy link
Contributor Author

Now this PR is ready, I'd like us to get started on the generation updates before we merge this pull request. @Kindest13 do you have all the information you need to get started in the kiota repository or do you want additional pointers?

Would be great if you can share more info on this

@baywet
Copy link
Member

baywet commented Jul 15, 2024

My guess is that since the ? is hardcoded here. And since only the properties need to be settable to null, we could probably hardcoded it here.
https://github.com/microsoft/kiota/blob/4d59461de79d1de58a309b4c25fee7b005a15ee5/src/Kiota.Builder/Writers/TypeScript/CodePropertyWriter.cs#L37

Let us know if you have additional questions.

@Kindest13
Copy link
Contributor Author

My guess is that since the ? is hardcoded here. And since only the properties need to be settable to null, we could probably hardcoded it here.

I would ask to describe in more details what should be done.
As I'm not sure what should be hardcoded it here

@baywet
Copy link
Member

baywet commented Jul 16, 2024

| null roughly

@Kindest13
Copy link
Contributor Author

Kindest13 commented Aug 7, 2024

| null roughly

Hey @baywet, is that correct?
So just this line?
writer.WriteLine($"{codeElement.Name.ToFirstCharacterLowerCase()}?: {returnType}{(isFlagEnum ? "[]" : string.Empty)} | null;");
microsoft/kiota#5106

@baywet
Copy link
Member

baywet commented Aug 8, 2024

yes it looks correct, however I left a couple of comments on the PR to wrap it up.

@baywet baywet dismissed stale reviews from andrueastman and themself via a94e896 August 13, 2024 14:45
@baywet baywet merged commit 0e10522 into microsoft:main Aug 13, 2024
6 checks passed
@baywet baywet mentioned this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Cannot write null to a nullable value
3 participants