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

Basic support of @deprecated trait in Smithy model #1570

Merged
merged 20 commits into from
Jul 28, 2022
Merged

Conversation

weihanglo
Copy link
Contributor

@weihanglo weihanglo commented Jul 25, 2022

Motivation and Context

#356

Basic support of [@deprecated] trait in Smithy model.

Description

Same feature parity between Smithy and Rust

The feature parity of @deprecated trait in Smithy model is exact the same as #[deprecated] attribute in Rust. Both have a since field to indicate when it has been deprecated, with an additional note/message field.

Service shapes are not supported

This patch mostly takes the implementation of documentation trait as a reference. Everywhere documentShape being called would get its counterpart of deprecatedShape, which is in exact our implementation of generating #[deprecated] attribute from @deprecated trait.

As a result, some important shapes haven't yet got support with documentation trait are also not going to get @deprecated feature. Namely, service, resource, and operation trait are not supported.

Support deprecated property in @enum trait

The other feature that is not actually a @deprecated trait but would start generating #[deprecated] attribute is @enum trait, which has an optional deprecated property is the same as deprecated trait in terms of functionality. This part resides in EnumGenerator.kt.

Testing

The simplest test is play around with @deprecated trait with different models.

@weihanglo weihanglo force-pushed the weihanglo-issue-356 branch 2 times, most recently from c6540f0 to 1ea10f8 Compare July 25, 2022 17:56
@github-actions

This comment was marked as outdated.

@82marbag
Copy link
Contributor

82marbag commented Jul 26, 2022

We should warn, not error out, on deprecated

@weihanglo
Copy link
Contributor Author

weihanglo commented Jul 26, 2022

We should warn, not error out, on deprecated

Reasonable to me.

However, #[deprecated] is at warn level as of 1.62.1. I suspect the failure was from clippy or something else turning on -D warnings or #![deny(warnings)]. Are we going to add #![allow(deprecated)] for each crate? I am fine with that.

@weihanglo weihanglo marked this pull request as ready for review July 26, 2022 09:04
@weihanglo weihanglo requested review from a team as code owners July 26, 2022 09:04
@82marbag
Copy link
Contributor

We should warn, not error out, on deprecated

Reasonable to me.

However, #[deprecated] is at warn level as of 1.62.1. I suspect the failure was from clippy or something else turning on -D warnings or #![deny(warnings)]. Are we going to add #![allow(deprecated)] for each crate? I am fine with that.

We are on 1.58 and we always remain behind in terms of Rust versions. Let's allow it and add a note to remove it when possible

Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

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

We need to add a changelog entry for this PR.

@weihanglo weihanglo force-pushed the weihanglo-issue-356 branch 2 times, most recently from 3cdd51c to 4061a1c Compare July 26, 2022 14:28
@smithy-lang smithy-lang deleted a comment from github-actions bot Jul 26, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Jul 26, 2022
@smithy-lang smithy-lang deleted a comment from github-actions bot Jul 26, 2022
@weihanglo weihanglo closed this Jul 26, 2022
@weihanglo weihanglo deleted the weihanglo-issue-356 branch July 26, 2022 14:54
@weihanglo weihanglo reopened this Jul 26, 2022
@github-actions

This comment was marked as outdated.

@crisidev crisidev added enhancement New feature or request needs-sdk-review labels Jul 26, 2022
@github-actions

This comment was marked as outdated.

* `Attribute.Custom.deprecated` is the main logic for building up
  `#[deprecated]` attribute
* `RustWriter.deprecatedShape` is the counterpart of `documentShape`,
  but we do not going to generalize it as what `documentShape` does.
  Deprecated is only for Rust code and probably won't be used in other
  output language.

Signed-off-by: Weihang Lo <[email protected]>
@github-actions

This comment was marked as outdated.

@weihanglo weihanglo requested a review from crisidev July 27, 2022 07:22
@github-actions

This comment was marked as outdated.

Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions

This comment was marked as outdated.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@weihanglo weihanglo enabled auto-merge (squash) July 28, 2022 18:42
@weihanglo weihanglo merged commit e38db43 into main Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants