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

Implement scalaImports trait #1550

Conversation

lenguyenthanh
Copy link
Contributor

@lenguyenthanh lenguyenthanh commented May 28, 2024

This is to implement scalaImports trait, which provides mechanism to add an import to a specific generated structure from smithy. The rationale behind is on this discussion.

I still need to check all the items below but want to make a draft PR first to gather some early feedback.

ps: You can see this pr in action here: lenguyenthanh/fide#100

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • [ ] Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • [ ] Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • [ ] Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@lenguyenthanh lenguyenthanh force-pushed the implement-scala-imports-trait branch from 0a7c0fe to ca34324 Compare May 28, 2024 09:09
@lenguyenthanh lenguyenthanh force-pushed the implement-scala-imports-trait branch from 85835cc to 8116bc0 Compare May 29, 2024 05:44
@Baccata
Copy link
Contributor

Baccata commented May 29, 2024

Looks good to me. The only other thing that I'd add would be some smoke tests, namely adding some specs in sampleSpecs folder that will be generated during the compilation of the bootstrapped module.

If you can reproduce a minimal spec showcasing the problem that led you to issue this PR, and how it can be fixed with this new trait, I'll deem the PR mergeable 👍

@lenguyenthanh
Copy link
Contributor Author

Looks good to me. The only other thing that I'd add would be some smoke tests, namely adding some specs in sampleSpecs folder that will be generated during the compilation of the bootstrapped module.

Thanks, I was looking at how to do smoke tests, but couldn't find. I'll add it today.

If you can reproduce a minimal spec showcasing the problem that led you to issue this PR, and how it can be fixed with this new trait, I'll deem the PR mergeable 👍

Should I write this in to a docs? probably 04-codegen/01-customisation/15-scala-imports.md?

@lenguyenthanh lenguyenthanh force-pushed the implement-scala-imports-trait branch from 11dff05 to 91b4542 Compare May 30, 2024 03:17
@lenguyenthanh lenguyenthanh force-pushed the implement-scala-imports-trait branch from 4b33ea7 to 3bfa92f Compare May 30, 2024 07:38
@lenguyenthanh
Copy link
Contributor Author

Added a minimal smoke test for this. And it also shows We can see why this We need this feature.

@scalaImports(["smithy4s.refined.Age.provider._"])
structure StructureWithScalaImports {
  @range(min: 13, max: 19)
  teenage: Age
}

This wouldn't compiled without the needed provider(which is delivered by scalaImports trait):

    implicit val rangeProvider: RefinementProvider.Simple[smithy.api.Range, Age] =
      RefinementProvider.rangeConstraint(x => x.value)

@lenguyenthanh
Copy link
Contributor Author

So, I think this is done code wise, now times for some documentation and change log. Is this okay better to do those in another PR and let this one going first?

@lenguyenthanh lenguyenthanh marked this pull request as ready for review May 30, 2024 08:40
@lenguyenthanh lenguyenthanh force-pushed the implement-scala-imports-trait branch from 9d27f20 to db55c28 Compare May 30, 2024 08:51
@Baccata
Copy link
Contributor

Baccata commented May 30, 2024

@lenguyenthanh I'd rather the docs were added to this PR, to be honest.

Should I write this in to a docs? probably 04-codegen/01-customisation/15-scala-imports.md?

This is a fine place, yes 👍

@lenguyenthanh
Copy link
Contributor Author

cool, I'll add it tomorrow.

@lenguyenthanh
Copy link
Contributor Author

Added documentation, and updated changelog. Tested by building the website locally. Please help me improve those docs 🙏

Screenshot 2024-05-31 at 12-28-31 Add Scala imports to generated code smithy4s

Copy link
Contributor

@Baccata Baccata left a comment

Choose a reason for hiding this comment

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

The documentation is great, thank you very much !

@Baccata Baccata merged commit 1790751 into disneystreaming:series/0.18 Jun 3, 2024
11 checks passed
@lenguyenthanh lenguyenthanh deleted the implement-scala-imports-trait branch June 3, 2024 14:28
@lenguyenthanh
Copy link
Contributor Author

thanks for your guidelines @Baccata . It was fun!

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

2 participants