Protocol in update lsp config #1610
Conversation
CHANGELOG.md
Outdated
| @@ -14,6 +14,7 @@ Thank you! | |||
| * Fixes an issue in which union members targetting Unit would fail to compile when used as traits (see [#1600](https://github.com/disneystreaming/smithy4s/pull/1600)). | |||
| * Make the `transform` method in generated `*Gen` algebras final. This should make it possible to derive e.g. `FunctorK` instances in cats-tagless automatically (see [#1588](https://github.com/disneystreaming/smithy4s/pull/1588)). | |||
| * Fixes commons.toKebabCase() sometimes drops the first letter (see [#1603](https://github.com/disneystreaming/smithy4s/pull/1603)). | |||
| * Added com.disneystreaming.smithy4s:smithy4s-protocol dependency to mill LSP plugin (see [#1610](https://github.com/disneystreaming/smithy4s/pull/1610)). | |||
There was a problem hiding this comment.
issue: it's not just Mill, the sbt plugin also benefits from this.
it's not exactly the LSP plugin either, just the codegen plugin. How about:
| * Added com.disneystreaming.smithy4s:smithy4s-protocol dependency to mill LSP plugin (see [#1610](https://github.com/disneystreaming/smithy4s/pull/1610)). | |
| * Addes `com.disneystreaming.smithy4s:smithy4s-protocol dependency` to the generation of `smithy-build.json` in the `smithy4sUpdateLspConfig` tasks of the codegen plugins (see [#1610](https://github.com/disneystreaming/smithy4s/pull/1610)). |
also, we'll need a new patch version - 0.18.25 is out.
There was a problem hiding this comment.
unresolving because of the 0.18.25 thing, just so that we don't forget
There was a problem hiding this comment.
I've pushed a merge with the latest of series/0.18
| @@ -2,23 +2,43 @@ import sbt.io.IO | |||
| val subproj = project | |||
|
|
|||
| val subproj2 = project.enablePlugins(Smithy4sCodegenPlugin) | |||
|
|
|||
| // "com.disneystreaming.smithy4s:smithy4s-protocol:${smithy4sVersion.value}" | |||
| val root = project | ||
| .in(file(".")) | ||
| .aggregate(subproj, subproj2) | ||
| .settings( | ||
| TaskKey[Unit]("checkSmithyBuild") := { | ||
| val generated = IO.readLines(file(".") / "smithy-build.json") | ||
| val expected = IO.readLines(file(".") / "expected.json") | ||
| val expected = s"""{ |
There was a problem hiding this comment.
suggestion: I see why you had to inline it (the smithy4sVersion variable part), but I'm not too happy about it.
What if we invented a simple templating language so that the JSON file could still be its own file, with a placeholder like
"com.disneystreaming.smithy4s:smithy4s-protocol:${SMITHY4S_VERSION}"and then we'd do a .replace("${SMITHY4S_VERSION}", smithy4sVersion.value) upon loading that string?
also, that file wasn't deleted in the PR so it probably still exists as a leftover.
There was a problem hiding this comment.
What would you think of changing from this string-to-string comparison to a json-to-json comparison?
The string-to-string business seems really brittle - even if the json gets loaded from a string in a file the json-to-json comparison would be less brittle.
json-to-json has a lot less magic, but if string-to-string is the standard then I won't rock the boat.
There was a problem hiding this comment.
I agree, we can stick to JSON comparisons on this level. If we care about formatting, we can test that in unit tests. I don't think we care about formatting much, though.
There was a problem hiding this comment.
What do you like for a json library? Circe?
There was a problem hiding this comment.
I just pushed a text version... I'll scratch at it a bit today, but this PR has been open more than a week. It's time to wrap it up.
modules/codegen/test/src/smithy4s/codegen/internals/SmithyBuildSpec.scala
Show resolved
Hide resolved
Co-authored-by: Jakub Kozłowski <[email protected]>
|
I think that's everything listed in the PR. Anything else? Otherwise it's ready to merge. |
|
thanks! |
PR Checklist (not all items are relevant to all PRs)