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 4796: Prepend global:: to references of generated C# types #4871

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Fix 4796: Prepend global:: to references of generated C# types #4871

merged 4 commits into from
Jun 21, 2024

Conversation

peterwurzinger
Copy link
Contributor

@peterwurzinger peterwurzinger commented Jun 20, 2024

This should fix #4796 by prepending global:: to references of generated C# types.

@deinok: Can you confirm if using a kiota build of this branch to generate your clients resolves the issue? I did some tests with samples that I had at hand and it seems to work, but having confirmation from a real-world example would be nice.

cc @baywet

@peterwurzinger peterwurzinger requested a review from a team as a code owner June 20, 2024 15:39
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 the contribution! Can you also add an entry to the changelog please? (unreleased, changed)

@deinok
Copy link

deinok commented Jun 20, 2024

Well, yes, it works more or less.

But now looks like a method called "AsList" does not exist.

image

@baywet
Copy link
Member

baywet commented Jun 20, 2024

@deinok this is because of #4823 Just update your abstractions reference

@peterwurzinger
Copy link
Contributor Author

peterwurzinger commented Jun 20, 2024

Hmm, I see.
At first sight it looks like it could be unrelated. Could you try using a build that's based on the current main branch and see if the problem occurs as well?

Also could you share the OpenAPI documents you're using, or are those internal ones?
I can have a look tomorrow morning if the issue was introduced by the PR - for today we probably are both watching Spain-Italy 😄

EDIT: Alright, seems like this is unrelated as per @baywet s comment

@deinok
Copy link

deinok commented Jun 20, 2024

Hmm, I see. At first sight it looks like it could be unrelated. Could you try using a build that's based on the current main branch and see if the problem occurs as well?

Also could you share the OpenAPI documents you're using, or are those internal ones? I can have a look tomorrow morning if the issue was introduced by the PR - for today we probably are both watching Spain-Italy 😄

EDIT: Alright, seems like this is unrelated as per @baywet s comment

Yup, unrelated, its the Remove LINQ change.

Regarding my OpenAPI file, its not the issue as it is. Probably its because i have a layout like this:
image
So they clash a bit internaly, thats the reason i need the global.

Regarding Spain-Italy, try not to drink too much and have fun 😄

EDIT: A test to check this kind of project layout where two folders contain two kiota instalations would be awesome

@peterwurzinger
Copy link
Contributor Author

peterwurzinger commented Jun 21, 2024

@baywet I added a changelog entry and a dedicated test for the global:: prefix to make sure this does not see any regression.

@deinok Well, Spain would have deserved more goals. 😄
I don't really have more time to create a unit test for that scenario. As far as I can tell, it would become quite complex, as the test assertion would boil down to "does it compile?" after the generation run(s).

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 making the change

@baywet baywet enabled auto-merge June 21, 2024 14:46
@baywet baywet merged commit 40e1196 into microsoft:main Jun 21, 2024
206 checks passed
@peterwurzinger peterwurzinger deleted the fix/4796 branch June 24, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full namespace Issue
3 participants