microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.88k stars 199 forks source link

File-diff between versions is impossible after GeneratedCode attribute with version is applied #5489

Open Balkoth opened 1 week ago

Balkoth commented 1 week ago

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Windows executable

Client library/SDK language

Csharp

Describe the bug

Since https://github.com/microsoft/kiota/issues/4907#issue-2381700619 an attribute like [global::System.CodeDom.Compiler.GeneratedCode("Kiota", "1.18.0")] is applied to all the generated classes.

Now an update of the generated files, touches and modifies every file. Good luck spotting and verifying real changes from this noise.

Expected behavior

Generated files should not change for the sake of changing.

How to reproduce

Generate a kiota client with v116 and then v117 and watch every file has changed.

Open API description file

No response

Kiota Version

1.16.0

Latest Kiota version known to work for scenario above?(Not required)

1.15.0

Known Workarounds

No response

Configuration

No response

Debug output

Click to expand log ``` ```

Other information

No response

baywet commented 1 week ago

Hi @Balkoth Thank you for using kiota and for reaching out.

This is something we've also noticed on the Microsoft Graph generated libraries. It adds a lot of noise. I've been looking at this attribute, and I don't understand why they shipped a single constructor making the parameters mandatory, when no validation is performed, and values are nullable. I suspect it was simply an oversight. So I've opened a PR on the runtime itself to get some feedback about that https://github.com/dotnet/runtime/pull/108393 But even if this gets merged and makes it to net9, it won't be available for earlier versions of the SDK. The dotnet team might be using that for telemetry purposes, or to display error messages somewhere.

As for kiota itself, there's not much value in having the version here. I think we could always set it to null to reduce the noise.

I'd like @andrueastman opinion on this one.

martincostello commented 1 week ago

But even if this gets merged and makes it to net9, it won't be available for earlier versions of the SDK.

I think at this stage in their release cycle, any additional constructor added would be part of .NET 10.

For me, the version is useful information because it tells tools consuming the compiled code via reflection what version of tool generated that code.

That doesn't take away from the fact that the changes might be noisy if there's no other changes to the compiled code, but the value is to the consumer of the generated code, not necessarily the consumer of Kiota itself (i.e. the person who generated that code).

Maybe just make the version Kiota writes out configurable?

Balkoth commented 1 week ago

I can see where you are coming from and understand that you would like to have this information somewhere in the resulting binary to obtain.

I just think that in this case it adds so much noise that it is not reasonable to include this information in the GeneratedCode attribute. Just updating the kiota version and regenerating the client, even if the description is still the same, will modify all the generated files. No one in their right mind can and will diff out the changes when generating a client the size of Microsoft Graph.

I don't know about anyone else, or if it is just me that is this pedantic when it comes to changes from generated code, but i like to know what goes into my libraries when i do stuff like this. Otherwise, this will fall back on me, when something suddenly stops working.

baywet commented 1 week ago

Thank you for the additional information.

For me, the version is useful information because it tells tools consuming the compiled code via reflection what version of tool generated that code.

@martincostello can you expand on how you get the version information and what you do with it?

I don't know about anyone else, or if it is just me that is this pedantic when it comes to changes from generated code

@Balkoth the usefulness of reviewing generated code could be argued both ways. In our case we have to result to filters in the diff, which makes for a more complex review, doesn't work with all the tools, etc... Lately we've also setup tooling to look only at the public API changes for Microsoft Graph. I'm not saying this is what everyone will do, just that it'd possible.

Maybe just make the version Kiota writes out configurable?

This is a solution we're unlikely to accept unless we get overwhelming feedback for that. We're trying to keep the CLI simple, and adding language specific switches goes against that design principle.

martincostello commented 6 days ago

can you expand on how you get the version information and what you do with it?

I'm not personally doing anything with the information - the reason I added it in the first place was to use the presence of the well-known attribute type for excluding generated code from code coverage tools.

My comment was more about what the intended usage of the data encoded in the attribute is in the general case.

Maybe instead of writing the exact version, Kiota instead writes out just the major version (1.0.0)?

baywet commented 6 days ago

Maybe instead of writing the exact version, Kiota instead writes out just the major version (1.0.0)?

That's an excellent suggestion in my view. It'd likely that from one major version to another we'd have A LOT of code change anyway. But code changes between minors/patches are hopefully limited. And it does not require an additional switch, etc...

Is this something you'd like to submit a pull request for provided some guidance?