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

Refactor TS code snippet to interfaces #1008

Merged
merged 14 commits into from
Jun 7, 2022
Merged

Refactor TS code snippet to interfaces #1008

merged 14 commits into from
Jun 7, 2022

Conversation

rkodev
Copy link
Contributor

@rkodev rkodev commented May 20, 2022

Refactor TS snippet generation to use Models and Interfaces.

Also resolves the issues listed below

Resolves #1016
Resolves #1013
Resolves #1014
Resolves #1015
Resolves #1012
Resolves #1011
Resolves #985
Resolves #965
Resolves #966
Resolves #814
Resolves #964

@rkodev rkodev marked this pull request as ready for review May 30, 2022 14:38
@rkodev rkodev marked this pull request as draft May 30, 2022 14:39
@rkodev rkodev marked this pull request as ready for review May 30, 2022 16: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 taking in the changes, a couple of additional comments.

Co-authored-by: Vincent Biret <vibiret@microsoft.com>
@baywet
Copy link
Member

baywet commented May 31, 2022

thanks for making the changes. I do have one more "discussion" point.
The changes you made with the SnippetCodeGraph should be generic enough and beneficial enough to be used for other kiota-based languages snippet generation.
I wonder whether we should invest in this pull request and make the changes to the other generators. How much would do you think this would take you? Is this something that would interest you?

@rkodev
Copy link
Contributor Author

rkodev commented May 31, 2022

@baywet Yes. That was the next step that I was thinking of. We can add all the changes to this PR for both Go and PowerShell, was afraid might be too large for 1 PR but I'm happy to add both to this

@baywet
Copy link
Member

baywet commented May 31, 2022

I'd vote to go ahead with that change, which should also include CSharp and Java

Copy link
Contributor

@zengin zengin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a few minor comments.


namespace CodeSnippetsReflection.OpenAPI.Test
{
public static class AssertUtil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zengin
Copy link
Contributor

zengin commented May 31, 2022

We have been waiting on this implementation for a while now to clear the TS tests. It would be nice if we can go ahead with the check-in and build other languages on top of this change gradually.

@rkodev rkodev force-pushed the feat/ts-interfaces branch from f8ab282 to 035fea4 Compare June 2, 2022 10:25
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

91.5% 91.5% Coverage
0.0% 0.0% Duplication

@baywet
Copy link
Member

baywet commented Jun 2, 2022

Alright, thanks for the input @zengin. @rkodev would you mind creating an issue to follow up on that please? And feel free to assign it to yourself and start tackling it right after this PR gets merged.

@rkodev
Copy link
Contributor Author

rkodev commented Jun 7, 2022

@baywet This is the issue
#1018

@rkodev rkodev merged commit 211879b into dev Jun 7, 2022
@rkodev rkodev deleted the feat/ts-interfaces branch June 7, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants