-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[Typescript-Node] Mark deprecated endpoints #19770
[Typescript-Node] Mark deprecated endpoints #19770
Conversation
assertNotNull(lines); | ||
|
||
String target = lines.get(lineNumber - 1); | ||
assertTrue(target.contains(line), "File does not contain line [" + line + "] at lineNumber [" + lineNumber + "]"); |
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.
If we make this a:
assertTrue(target.contains(line), "File does not contain line [" + line + "] at lineNumber [" + lineNumber + "]"); | |
assertTrue(target.contains(line), "Expected "+ path + ":" + lineNumber + " to contain [" + line + "]"); |
Then links become clickable in the command line
//files.forEach(File::deleteOnExit); | ||
|
||
TestUtils.assertFileContains(Paths.get(output + "/api/defaultApi.ts"), | ||
"* @deprecated", 118); |
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.
asserting on the line number feels very fragile to me. Any refactor in the mustache that changes any lines before the deprecated tag will require future contributors to update this assertion.
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.
Agree. Now that I think about it, it isn't a great idea. I have removed it.
final ClientOptInput clientOptInput = configurator.toClientOptInput(); | ||
DefaultGenerator generator = new DefaultGenerator(); | ||
List<File> files = generator.opts(clientOptInput).generate(); | ||
//files.forEach(File::deleteOnExit); |
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.
//files.forEach(File::deleteOnExit); |
?
or put it in a finally
block?
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.
I have missed it. It is not fixed.
modules/openapi-generator/src/main/resources/typescript-node/api-single.mustache
Outdated
Show resolved
Hide resolved
@@ -114,6 +114,8 @@ export class DefaultApi { | |||
* @param strCode Code as header | |||
* @param strCode2 Code as header2 | |||
* @param patchUsersUserIdRequest Patch user properties to update. | |||
* | |||
* @deprecated |
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.
see above
…pi-single.mustache Co-authored-by: Joscha Feth <[email protected]>
@@ -155,7 +155,7 @@ export class {{classname}} { | |||
{{/allParams}} | |||
{{#isDeprecated}} | |||
* | |||
* @deprecated | |||
* @deprecated |
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.
I think you had tabs. If you use spaces to indent it, it should work. My suggestion removed the tab.
@@ -153,6 +153,10 @@ export class {{classname}} { | |||
{{#allParams}} | |||
* @param {{paramName}} {{description}} | |||
{{/allParams}} | |||
{{#isDeprecated}} | |||
* | |||
* @deprecated |
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.
* @deprecated | |
* @deprecated |
This should work, it's tricky in the webinterface :)
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.
🎉
@gcatanese can you please re-generate the samples? then we can merge |
diff seems unrelated to this PR: |
5f345c0 was just merged, which fixes the samples |
@gcatanese can you rebase on the latest master branch? |
38c5618 doesn't look quite right. You have to either fast forward or rebase your changes on top, so you don't get the diff between this branch and the merge target (master). If you revert the last commit and force push, then github should actually show an |
38c5618
to
d68656a
Compare
Add @deprecated tag when processing deprecated operations in the OpenAPI file
@wing328 I have added a helper method (
assertFileContains(Path path, String line, int lineNumber)
) to verify a specific line (more effective than searching for the string in the file anywhere), let me know if this is ok or you see some drawbacks. Thanks.PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10)