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

Add @vector and @indexedSeq #303

Merged
merged 4 commits into from
Jul 12, 2022
Merged

Add @vector and @indexedSeq #303

merged 4 commits into from
Jul 12, 2022

Conversation

daddykotex
Copy link
Contributor

@daddykotex daddykotex commented Jul 11, 2022

Adds a mechanism for users to customise the collection type used when generating Scala code from smithy list shapes.

Users can now opt-in Vector or IndexedSeq. List remains the default collection type.

Both annotations can only be added to list since
`set` is already specialized. They can't be mixed with
each other or `uniqueItems`

`@uniqueItems` is deprecated but I decided to support it
nonetheless because 2.0 is not out yet
@daddykotex
Copy link
Contributor Author

note: CI does not run because I don't target main I guess?

@@ -607,7 +630,7 @@ private[codegen] class SmithyToIR(model: Model, namespace: String) {
enumDef.getName().asScala
)
// List
case (N.ArrayNode(list), Type.List(mem)) =>
case (N.ArrayNode(list), Type.List(mem, _)) => // todo sure ?
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just ignoring the hints because you don't need them here? That seems fine to me


#### Specialized collection types

Smithy supports `list` and `set`, Smithy4s renders that to `List[A]` and `Set[A]` respectively. You can also use the `@uniqueItems` annotation on `list` which is equivalent to `set`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a note that uniqueItems is going away or remove that comment entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

it is actually not going away. The set keyword is going away in favour of uniqueItems (aws has pivoted on this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I read that from the source and I thought it was the latest state


Smithy supports `list` and `set`, Smithy4s renders that to `List[A]` and `Set[A]` respectively. You can also use the `@uniqueItems` annotation on `list` which is equivalent to `set`.

Smithy4s has support for two specialized collection type: `Vector` and `IndexedSeq`. The following examples shows how to use them:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Smithy4s has support for two specialized collection type: `Vector` and `IndexedSeq`. The following examples shows how to use them:
Smithy4s has support for two specialized collection types: `Vector` and `IndexedSeq`. The following examples show how to use them:

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in b3365de

}
```

Both annotation are only available on `list`. You can't mix `@vector` with `@indexedSeq`, nor `@uniqueItems`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Both annotation are only available on `list`. You can't mix `@vector` with `@indexedSeq`, nor `@uniqueItems`.
Both annotations are only available on `list`. You can't mix `@vector` with `@indexedSeq`, and neither one can be used with `@uniqueItems`.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in b3365de

Comment on lines 4 to 5
use smithy4s.meta#vector
use smithy4s.meta#indexedSeq
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these used in this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in b3365de

@@ -650,8 +650,15 @@ private[codegen] class Renderer(compilationUnit: CompilationUnit) { self =>
implicit class TypeExt(tpe: Type) {
def schemaRef: Line = tpe match {
case Type.PrimitiveType(p) => Line(schemaRefP(p))
case Type.List(member) => line"list(${member.schemaRef})"
case Type.Set(member) => line"set(${member.schemaRef})"
case Type.List(member, hints) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the wrong level to put this. Either you should have Type.Collection(tag, member), where tag is an enumeration, or you should have Type.List, Type.Vector, Type.IndexedSeq

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in b3365de

@@ -24,3 +24,16 @@ structure packedInputs {}
@trait(selector: "structure :not([trait|error])")
@idRef(failWhenMissing: true, selector: "union")
string adtMember

@trait(selector: """
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some documentation for these annotations, in the form of a triple-slash comment (like for @adtMember)

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in b3365de

Base automatically changed from dfrancoeur/kill-schematic to main July 11, 2022 22:02
@Baccata Baccata merged commit 2d07dba into main Jul 12, 2022
@Baccata Baccata deleted the dfrancoeur/collections-pt2 branch July 12, 2022 12:05
@@ -195,6 +191,14 @@ object Type {
case class PrimitiveType(prim: Primitive) extends Type
}

sealed abstract class CollectionType(val tpe: String)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to sum it up: I went the easy route lol

sorry for that, I could have come up with this solution on the first try

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.

3 participants