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

Render deprecation hints #599

Merged
merged 13 commits into from
Nov 20, 2022
Merged

Render deprecation hints #599

merged 13 commits into from
Nov 20, 2022

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Nov 12, 2022

Made a separate commit for the actual implementation so that you can see how it affects the deprecated shapes:

files in modules/example of this commit

I think this doesn't have to go into 0.17 as it wouldn't make breaking changes.

Edit: about the above, I think this might be source breaking in a way (people might have fatal warnings on). Let's stick it into 0.17.

Solves the deprecation part of #256

build.sbt Show resolved Hide resolved
modules/codegen/src/smithy4s/codegen/IR.scala Show resolved Hide resolved
case _ => Lines.empty
}

private def deprecationAnnotation(hints: List[Hint]): Line = {
Copy link
Member Author

Choose a reason for hiding this comment

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

the implementation feels a bit dirty, suggestions welcome

Copy link
Contributor

Choose a reason for hiding this comment

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

the only thing I'd change here is have an extra map, before the that's already there where I call renderStringLiteral before building the line to avoid ${renderStringLiteral(msg)}

Copy link
Member Author

Choose a reason for hiding this comment

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

tbh I actually like that renderStringLiteral is being interpolated (as long as it doesn't break the line)

@@ -166,6 +185,7 @@ private[codegen] class Renderer(compilationUnit: CompilationUnit) { self =>
val name = s.name
val nameGen = NameRef(s"${name}Gen")
lines(
deprecationAnnotation(s.hints),
Copy link
Member Author

Choose a reason for hiding this comment

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

Thought it would make sense to deprecate just the service trait and type alias, but we could deprecate the companion object / val alias as well. I don't have a strong opinion on this.

@@ -273,7 +273,8 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
value.getName().asScala,
value.getValue,
index
)
),
hints = Nil
Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK 1.x enums don't allow hints on values.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is correct

modules/codegen/src/smithy4s/codegen/SmithyToIR.scala Outdated Show resolved Hide resolved
Comment on lines 308 to +312
val values = shape
.getEnumValues()
.asScala
.map { case (name, value) =>
EnumValue(name, value, name)
val member = shape.getMember(name).get()
Copy link
Member Author

Choose a reason for hiding this comment

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

feels a bit odd to do the lookup like this, but it's not that much repetition and it should be safe at all times.

Comment on lines 111 to 112
// Ignores warnings in code using the deprecated Enum trait.
scalacOptions += "-Wconf:msg=object Enum in package api is deprecated:silent"
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI here's what Enum now generates as:

@deprecated(message = "The enum trait is replaced by the enum shape in Smithy 2.0", since = "2.0")
object Enum extends Newtype[List[EnumDefinition]] {
  val id: ShapeId = ShapeId("smithy.api", "enum")
  val hints : Hints = Hints(
    smithy.api.Documentation("Constrains the acceptable values of a string to a fixed set\nof constant values."),
    smithy.api.Trait(selector = Some("string :not(enum)"), structurallyExclusive = None, conflicts = None, breakingChanges = Some(List(smithy.api.TraitDiffRule(change = smithy.api.TraitChangeType.PRESENCE.widen, severity = smithy.api.TraitChangeSeverity.ERROR.widen, path = None, message = None)))),
    smithy.api.Deprecated(message = Some("The enum trait is replaced by the enum shape in Smithy 2.0"), since = Some("2.0")),
  )
  val underlyingSchema : Schema[List[EnumDefinition]] = list(EnumDefinition.schema).withId(id).addHints(hints).validated(smithy.api.Length(min = Some(1), max = None))
  implicit val schema : Schema[Enum] = recursive(bijection(underlyingSchema, asBijection))
}

@kubukoz
Copy link
Member Author

kubukoz commented Nov 12, 2022

I'll get back to this after the weekend, most likely

@kubukoz kubukoz marked this pull request as ready for review November 17, 2022 23:11
@kubukoz
Copy link
Member Author

kubukoz commented Nov 17, 2022

@Baccata I think this is ready to go from my side

@@ -988,4 +1023,15 @@ private[codegen] class Renderer(compilationUnit: CompilationUnit) { self =>
case _ => _ => line"null"
}

private def renderStringLiteral(raw: String): Line = {
import scala.reflect.runtime.universe._
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the import used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Literal and Constant

import smithy4s.schema.Schema.struct
import smithy4s.ShapeTag

@deprecated(message = "A compelling reason", since = "0.0.1")
Copy link
Contributor

Choose a reason for hiding this comment

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

hahaha

Copy link
Contributor

@daddykotex daddykotex left a comment

Choose a reason for hiding this comment

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

great job, it's more work that I had anticipated
kudos on the work on dynamic, this PR serves as a great reference if for changes that would need to apply there as well

@@ -742,6 +740,8 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
// traits from the synthetic namespace, e.g. smithy.synthetic.enum
// don't have shapes in the model - so we can't generate hints for them.
.filterNot(_.toShapeId().getNamespace() == "smithy.synthetic")
// enumValue can be derived from enum schemas anyway, so we're removing it from hints
.filterNot(_.toShapeId().toString() == "smithy.api#enumValue")
Copy link
Contributor

Choose a reason for hiding this comment

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

thy shall not use .toString when there is an alternative (in this case, ShapeId.fromParts("smithy.api", "enumValue") or probably even EnumValueTrait.ID or something

Copy link
Member Author

Choose a reason for hiding this comment

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

now you're just nitpicking :P

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, just .toString trauma 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

:heavy-nodding:

@kubukoz kubukoz merged commit b1246cc into series/0.17 Nov 20, 2022
@kubukoz kubukoz deleted the render-deprecation branch November 20, 2022 00:21
@Baccata Baccata added this to the 0.17.0 milestone Nov 23, 2022
@zetashift zetashift mentioned this pull request Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants