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

remove core dependency auto-add from sbt plugin #38

Merged
merged 1 commit into from
Jan 12, 2022
Merged

remove core dependency auto-add from sbt plugin #38

merged 1 commit into from
Jan 12, 2022

Conversation

lewisjkl
Copy link
Contributor

Closes #34.

The issue was that the sbt plugin was auto-adding the smithy4s-core dependency. This worked in all cases except for Scala JS since that would require the use of %%%. We could have updated the plugin to use %%%, but that would require the Scala JS SBT plugin be present on all projects using it, which we do not want to require.

@lewisjkl lewisjkl requested review from Baccata and kubukoz January 11, 2022 19:32
@@ -30,9 +30,11 @@ import smithy4s.codegen.Smithy4sCodegenPlugin
val myModule = project
.in(file("modules/my-module"))
.enablePlugins(Smithy4sCodegenPlugin)
// version for smithy4s-core is sourced from Smithy4sCodegenPlugin
.settings(libraryDependencies += "com.disneystreaming.smithy4s" %%% "smithy4s-core" % smithy4sVersion.value)
Copy link
Member

Choose a reason for hiding this comment

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

%%% won't work on builds without scalajs. In general, I think this approach is non-ideal (asking people to add the dependency themselves) but I don't have a better idea for now :/ hoping @Baccata has something better

Copy link
Member

Choose a reason for hiding this comment

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

ah, I now see the comment in #34 saying it's already been discussed

Copy link
Member

Choose a reason for hiding this comment

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

still, the recommendation should be %% I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I was trying to signal that it worked for ScalaJS as well, but maybe that is implied enough to change. And it is likely that ScalaJS folks will figure that out where a newcomer who isn't familiar with '%%%' might get confused

Copy link
Member

Choose a reason for hiding this comment

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

💯

Copy link
Member

@kubukoz kubukoz left a comment

Choose a reason for hiding this comment

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

This will probably need an entry in the release notes :)

@kubukoz kubukoz merged commit bc038c2 into main Jan 12, 2022
@Baccata Baccata deleted the issue-34 branch January 12, 2022 08:09
@armanbilge
Copy link
Contributor

We could have updated the plugin to use %%%, but that would require the Scala JS SBT plugin be present on all projects using it, which we do not want to require.

This ship has probably sailed, but FYI %%% is a separate plugin.
https://github.com/portable-scala/sbt-platform-deps

@Baccata
Copy link
Contributor

Baccata commented Apr 8, 2022

#38 (comment)

I think we might be able to revert then.

Baccata pushed a commit that referenced this pull request May 10, 2022
remove core dependency auto-add from sbt plugin
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.

schematic.StubSchematic can't be used in Scala.JS
4 participants