Comments (15)
Ah sorry, I missed to prepend that in the initial PR - my bad.
Just one thing to note regarding implementing the fix:
From the top of my head I would say, that initializing the StringBuilder with that keyword would result in it being placed at the end of the resulting fullname, as the type hierarchy is being traversed bottom-up and names are prepended instead of appended.
Imo it should be inserted here:
with
fullNameBuilder.Insert(0, $"global::{codeNamespace.Name}.");
from kiota.
Since this was a regression to some extent [...]
Yeah I agree, it should have been included in the initial PR right away, I'll create a separate one to fix this issue.
from kiota.
Thanks for raising this @deinok
Any chance you'd be willing to submit a PR for this? (cc @peterwurzinger) I believe you'd simply need to
-
make sure the prefix exists when the stringBuilder is initialized here
-
Update the relevant tests
-
Update samples similar to PR at microsoft/kiota-samples#5824
from kiota.
How about the other obvious issue that's visible in that picture? The code is completely unreadable, which was caused by recent Kiota changes, adding more prefixes will certainly not help that.
from kiota.
Before we make any further change, I'd like the input of @maisarissi and @sebastienlevert
Original behaviour: only use the fully qualified namespace when conflicts would happen. Worked well besides edge cases identified in #4475
New behaviour since #4751 : always emit the FQDN. We've received feedback this makes the code harder to read. And it still results in conflicts in some cases (this issue).
The question being: which tradeoff would you prefer between readability and reliability?
from kiota.
hey @baywet . Per what I could understand there is no other way we can identify whether the namespace will conflict with a type. Is that correct? Because they initial error was all about that scenario.
from kiota.
I think there are a couple of avenues we can take here:
- keep FQDN prefix all type references with global:: (might be a problem for inner classes for example, but we could special case that one)
- revert the FQDN change, and try to iron out the logic that decides whether or not to use FQDN when conflicts might happen.
The challenge with option 2 is that the symbols resolution in CSharp is not clear (and changed slightly when we moved to roselyn as far as I can tell). The rules of thumb of when a symbol might conflict are roughly "whenever is present as a segment of a namespace, as a symbol directly under a imported namespace, as a member name for the current type" and there are probably missing rules here which is why we've had to review the implementation multiple times already.
from kiota.
How about the other obvious issue that's visible in that picture? The code is completely unreadable, which was caused by recent Kiota changes, adding more prefixes will certainly not help that.
Thats another issue, but if it is going in the verbose direction, we need it fully functional and correct. And the only correct way is using the global directive.
from kiota.
I think there are a couple of avenues we can take here:
- keep FQDN prefix all type references with global:: (might be a problem for inner classes for example, but we could special case that one)
- revert the FQDN change, and try to iron out the logic that decides whether or not to use FQDN when conflicts might happen.
The challenge with option 2 is that the symbols resolution in CSharp is not clear (and changed slightly when we moved to roselyn as far as I can tell). The rules of thumb of when a symbol might conflict are roughly "whenever is present as a segment of a namespace, as a symbol directly under a imported namespace, as a member name for the current type" and there are probably missing rules here which is why we've had to review the implementation multiple times already.
regarding the option 1, I would ask the roslyn team on how to better respresent that and how they do it internally. They probably have much more experience in this kind of issues and can provide insights
Regarding on what to choose, yes, option 1 is pretty ugly, as same as ugly as looking at MSIL code. It's generated code and should not be touched by hand, so I really dont care about verbosity, but probably option 1 will give you more flexibility in the future, and you can always try to move to path 2 in the future
But im going to say it, if kiota could have a SourceGenerator, i could fix most of the issues about Verbosity as user will not see it in the git, exactly as MSIL is not "shown" in git
from kiota.
Thanks for the additional context. For everybody's information, we've explored source generators in the past. But the netstandard2.0 limitation had serious performance impacts. Plus our own experience building some of them (updating versions automatically and default settings) has been far from great at this point (debugging and whatnot).
Since there has been no overwhelming demand for them from the community, we didn't pursue that avenue further.
I guess the ask for @maisarissi and the community here is roughly: "do we care about generated code readability?" Since our recommendation is that it shouldn't be edited anyway, I lean on "no". I do understand that in some debug scenarios, having access to code that's easier to read makes for a better debugging experience. At that point however, how much could we rely on linting tools and quick formatting to make the experience better? (i.e. is it a documentation issue? are we missing a section "if you want to debug through the code, run this first"? )
from kiota.
@baywet Should we fix this issue by prepending the global namespace? While there still might be an ongoing discussion regarding the potential need for a more sophisticated namespace collision detection, it wouldn't really matter if there was a global::
prefix or not. I mean, I was apparently wrong in sharing your assumption of negligible readability of the generated code, but it wouldn't make things much "worse".
from kiota.
Alright, I'd have appreciated for @maisarissi to make a decision on this so we don't do work we'd have to potentially undo later on.
I logged a separate docs issue MicrosoftDocs/openapi-docs#99
Since this was a regression to some extent (at least for some scenarios), I'm ok with adding the global prefix. Even though that's going to ruffle some people's feathers. We can always undo the change later when a decision on the topic is made.
Are you willing to submit a pull request for that?
from kiota.
Regarding the topic of readability. While we will always try to make it as readable as possible, there will always be cases where machine-generated code will need to take a path that impacts the readability of the code in favor of better generation. There are areas where the code is pretty much very readable, where it's a bit less because of the constraints. I don't think we should be peeking into the generated code and we should leave it as is. I see this code to be as opaque as can be while offering really easy debugging and learning experience.
So my stance: We should not tax any of our development efforts to make the code "more readable" at the expense of complex solutions.
from kiota.
I'm in the same page as Sebastien.
Even tough it would be awesome to always have as much readable code as possible, having the best generation possible should be our goal. Readability is a plus whenever possible and when it don't come with perf or generation costs.
from kiota.
I think prefixing with global::
is the way to go here:
#5020
from kiota.
Related Issues (20)
- description_for_human in ai-plugin.json file generated by Kiota contains speciall characters \u0026apos; HOT 1
- Kiota plugin should not generate function for API with request body contains nested object HOT 1
- Kiota should not generate plugin when selected APIs contain unsupported authorization scheme HOT 1
- When generate API plugin, kiota should make sure that server url inside OpenAPI spec file using https protocol HOT 1
- The plugin manifest should be created in a temp folder. HOT 2
- Kiota fails when creating a plugin for https://api.github.com/ GET operation HOT 5
- When generating the plugin manifest, Kiota should default to OpenAPI path descriptions when creating function descriptions
- Add conversation starters as part of the plugin manifest
- Kiota should warn the user when re-generating that all changes did manually by them will be overridden
- Ruby generation segmentation fault from Azure DevOps Services Git swagger file HOT 6
- Error models with a property called message no longer generate correctly HOT 2
- Documentation for Handler middleware (all languages) HOT 7
- Update info command to show bundle dependencies HOT 3
- missing UntypedNode import in Java, in various places HOT 4
- Kiota plugin edit does not update plugin if file extension has not changed HOT 5
- add github branch policy definition
- Schema name conflict causing getter and setter ( {schemaName}_{propertyName}able ) interfaces to not be generated HOT 2
- Stack overflow Segmentation failure error with v1.18 when class presents references to itself HOT 3
- TypeScript - anyOf/oneOf arrays do not emit the correct serialization and deserialization information HOT 7
- PHP - anyOf/oneOf primitive type array serialization wrong
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from kiota.