Skip to content

Handle null values for collections in samples#7349

Merged
srnagar merged 2 commits intomicrosoft:mainfrom
srnagar:sample-null-collection
May 15, 2025
Merged

Handle null values for collections in samples#7349
srnagar merged 2 commits intomicrosoft:mainfrom
srnagar:sample-null-collection

Conversation

@srnagar
Copy link
Member

@srnagar srnagar commented May 15, 2025

No description provided.

@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:client:java Issue for the Java client emitter: @typespec/http-client-java label May 15, 2025
@github-actions
Copy link
Contributor

No changes needing a change description found.

@srnagar srnagar enabled auto-merge May 15, 2025 00:28
@weidongxu-microsoft
Copy link
Contributor

I see, for autorest, origin code is passable, it logs error but continue with the empty List/Map.

In typespec, emitter converted the error log to error in diagnostic, and then compiler reports the error.

Let us see how it works in mgmt (many JSON example is not accurate, legacy problem). If there is too many other kind of type error on it, we may lower it to "log warning" than exception.

@srnagar
Copy link
Member Author

srnagar commented May 15, 2025

It would be better to fix the samples or disable sample generation than to log warnings and ignore as that would result in bad samples getting generated.

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 🛝 VSCode Extension

@XiaofeiCao
Copy link
Member

I see, for autorest, origin code is passable, it logs error but continue with the empty List/Map.

In typespec, emitter converted the error log to error in diagnostic, and then compiler reports the error.

Let us see how it works in mgmt (many JSON example is not accurate, legacy problem). If there is too many other kind of type error on it, we may lower it to "log warning" than exception.

I'm not against it. Just for my understanding, will this be a case that "error log affects the emitter result"? Should we consider error logs failure for codegen?

@srnagar srnagar added this pull request to the merge queue May 15, 2025
Merged via the queue into microsoft:main with commit 4ef8edf May 15, 2025
25 checks passed
@srnagar srnagar deleted the sample-null-collection branch May 15, 2025 01:40
@weidongxu-microsoft
Copy link
Contributor

I'm not against it. Just for my understanding, will this be a case that "error log affects the emitter result"? Should we consider error logs failure for codegen?

There is code in emitter.ts to report warn/error from logging to diagnostic.
https://github.com/microsoft/typespec/blob/main/packages/http-client-java/emitter/src/emitter.ts#L170-L179

We log error in autorest, to say "this is error in JSON exmaple", but ignore that since it only affect test/sample. I think it is still reasonable.
(if we want failure, we'd throw like Srikanta did here)

With the code in emitter.ts, we seems had to use log.warn in this particular case, to avoid fail on test/sample value (even if they are invalid...).

@weidongxu-microsoft
Copy link
Contributor

It would be better to fix the samples or disable sample generation than to log warnings and ignore as that would result in bad samples getting generated.

Yeah, that is the preference. Just mgmt had too many libs, and fixing them require contact service owner (we don't have a good list for contact).
And disable the whole sample/test may not be desirable, if only a few of them have problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

emitter:client:java Issue for the Java client emitter: @typespec/http-client-java

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants