Full namespace Issue about kiota HOT 15 CLOSED

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:

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
  

  kiota/src/Kiota.Builder/Writers/CSharp/TypeDefinitionExtensions.cs

  Line 15 in 56566b2

  return AppendTypeName(typeDefinition, fullNameBuilder).ToString();

• 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:

1. keep FQDN prefix all type references with global:: (might be a problem for inner classes for example, but we could special case that one)
2. 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:

1. keep FQDN prefix all type references with global:: (might be a problem for inner classes for example, but we could special case that one)
2. 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.