-
Notifications
You must be signed in to change notification settings - Fork 192
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
feat: generate code deprecation tags #1006
Conversation
Codegen Tests 1 files 18 suites 27s ⏱️ Results for commit 774c499. |
APIX Tests 1 files 78 suites 3m 39s ⏱️ Results for commit 774c499. |
Typescript Tests 7 files 76 suites 3m 55s ⏱️ Results for commit 774c499. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intentionally not regenerate TS-SDK and PY-SDK? Also, I didn't open examplesIndex.json
but it looks like a big change so just making sure that was intentional.
The python and codegen stuff LGTM! But I don't feel qualified to review on the other languages or apix. Happy to approve if someone else can speak for those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Explorer related changes lgtm. Happy to approve once the questions around the typescript and python SDKs are answered.
Codegen Tests 1 files 18 suites 27s ⏱️ Results for commit a938a0b. |
APIX Tests 1 files 78 suites 3m 19s ⏱️ Results for commit a938a0b. |
Python Tests 10 files 10 suites 2m 27s ⏱️ Results for commit a938a0b. |
Typescript Tests 7 files 76 suites 4m 18s ⏱️ Results for commit a938a0b. |
APIX Tests 1 files 78 suites 3m 36s ⏱️ Results for commit 40f821e. |
Codegen Tests 1 files 18 suites 28s ⏱️ Results for commit 40f821e. |
Typescript Tests 7 files 76 suites 3m 58s ⏱️ Results for commit 40f821e. |
Codegen Tests 1 files 18 suites 29s ⏱️ Results for commit e8eb24d. |
APIX Tests 1 files 78 suites 3m 27s ⏱️ Results for commit e8eb24d. |
Typescript Tests 7 files 76 suites 4m 14s ⏱️ Results for commit e8eb24d. |
Go Tests 5 files 5 suites 56s ⏱️ Results for commit 8bbe8cd. |
All SDKs have been updated where appropriate and regenerated. As mentioned in the PR description, the examples index is also updated. That should be something that runs as part of CI/CD or as part of the release process. And also whenever new examples are submitted. TODO |
Codegen Tests 1 files 18 suites 27s ⏱️ Results for commit 8bbe8cd. |
APIX Tests 1 files 78 suites 3m 46s ⏱️ Results for commit 8bbe8cd. |
Typescript Tests 7 files 76 suites 4m 9s ⏱️ Results for commit 8bbe8cd. |
I'm confused about why the generated csharp, go, kotlin, and swift SDKs all seem to have the |
I'll have to look at it again, because the deprecated tags should be there.
…On Tue, Mar 1, 2022, 7:45 AM Joel Dodge ***@***.***> wrote:
I'm confused about why the generated csharp, go, kotlin, and swift SDKs
all seem to have the deprecated stuff but typescript and python don't -
even though I see the required changes in their generators. I assume this
is still WIP @jkaster <https://github.com/jkaster> ?
—
Reply to this email directly, view it on GitHub
<#1006 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABORGAQR4T6YXIUXOIO5EQTU5Y3Q3ANCNFSM5PJGI2MQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@joeldodge79 that's because the specification with deprecated parameters won't be available until Looker 22.4 is released. This change added deprecated params to the test specs used for testing the generator. This PR is done after adding new deprecation tags for some of the generators, and has new unit tests that verify tagging deprecated params as well as method deprecation attributes where appropriate. TypeScript already had |
${this.deprecated(indent, method)}${indent}${this.jvmOverloads(method)}fun ${ | ||
method.name | ||
}( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was the source of my confusion. It's not related to deprecated method params - instead it's back-filling already existing functionality to deprecate a whole method. Typescript already had it and you added it in this PR kotlin, csharp, go, swift but not python.
When I saw deprecated
showing up in generated code for some languages and not others I got really confused.
I'm ok with this as is, but I feel like had this bullet point in your PR description been a different PR I would have gotten through this one a lot quicker.
adds deprecated attributes for relevant Kotlin, Swift, Go, and C# methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you add that description to the PR? I see it now. This PR was for adding deprecated tags where appropriate. Python has no deprecation attributes that I could find, so I didn't add deprecated tags to the method headers. This was to bring the other SDKs that support deprecated notation up to date with TypeScript, and add deprecation comments to method params for TypeScript, and to add deprecated comments for method parameters.
Go Tests 5 files 5 suites 1m 2s ⏱️ Results for commit 898b338. |
Codegen Tests 1 files 18 suites 24s ⏱️ Results for commit 898b338. |
APIX Tests 1 files 78 suites 3m 37s ⏱️ Results for commit 898b338. |
Python Tests 10 files 10 suites 2m 33s ⏱️ Results for commit 898b338. |
Typescript Tests 7 files 76 suites 4m 8s ⏱️ Results for commit 898b338. |
(DEPRECATED)
to deprecated method parameters that don't include the text "deprecated"