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

feat: adds @deprecated convenience decorator #4415

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

mschnee
Copy link
Contributor

@mschnee mschnee commented Jan 13, 2020

Partially implements: #4300
See: #4406

This has been split from #4406 as a more manageable, smaller PR.

Added

This PR adds @deprecated to Controller classes and methods, and marks all paths as deprecated: true in accordance with the OpenAPI Operation Object specification.

As the spec defines deprecated as an optional property, this feature will only add deprecated: true and never deprecated: false.

Not (yet) supported

This PR does not add support for marking parameters as deprecated

Examples

@deprecated()
class MyController {
   @get('/greet')
   public async function greet() {
    return 'Hello, World!'
  }

  @get('/greet-v2')
  @deprecated(false)
  public async function greetV2() {
    return 'Hello, World!'
  }
}

class MyOtherController {
  @get('/echo')
  @deprecated()
  public async function echo() {
    return 'Echo!'
  }
}

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • (n/a) Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@mschnee
Copy link
Contributor Author

mschnee commented Jan 13, 2020

Note: The Node 8 build failed due to a 404 from http://calculator-webservice.mybluemix.net/calculator?wsdl, which at the moment (2 hours later) is not returning a 404. Will trigger a new build.

@dhmlau dhmlau added the OpenAPI label Jan 14, 2020
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Thank you @mschnee for the contribution. The changes look good to me at high level, I'd like @jannyHou to review OpenAPI-specific parts and @raymondfeng to review how the decorator is defined using metadata APIs.

@bajtos bajtos requested a review from jannyHou January 24, 2020 16:04
@raymondfeng
Copy link
Contributor

raymondfeng commented Jan 24, 2020

My only concern is that @deprecated may confuse our developers as it's not obvious for OpenAPI spec only. See similar discussion at #4416 (comment).

@strongloop/loopback-maintainers What do you think?

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@mschnee Thank you so much for improving the OpenAPI series decorators! 🙇
I agree with @raymondfeng 's suggestion that uses a namespace to organize the operation decorators. Otherwise your code LGTM 👍

@dougal83
Copy link
Contributor

dougal83 commented Jan 29, 2020

As the spec defines deprecated as an optional property, this feature will only add deprecated: true and never deprecated: false.

Thinking about it, it wouldn't hurt to explicitly default to setting depreciated: false as the last property of a PathsObject. Considering the default is false it would be informational. Probably outside the scope of this PR.

@mschnee mschnee force-pushed the feat/deprecated-decorator branch 2 times, most recently from dd54f76 to adf9d77 Compare January 31, 2020 00:01
@mschnee
Copy link
Contributor Author

mschnee commented Jan 31, 2020

Squash will happen after all the tests are clean :)

@mschnee mschnee force-pushed the feat/deprecated-decorator branch from 9ba74ed to 7ed2872 Compare January 31, 2020 00:56
@raymondfeng raymondfeng merged commit 6b6b5f0 into loopbackio:master Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants