Support more HTTP methods, keep more details about HTTP matching failures #493
Conversation
|
http4s supports many more methods, maybe we should list them all? Or even support arbitrary strings for method names and try to parse them into http4s at derivation time? Smithy itself doesn't validate the string. |
|
@kubukoz yeah we could make it an open-enumeration maybe ? Have a catch-all |
|
sounds alright - maybe I'll remove the ones that we don't explicitly use and move them into the open. |
Did it for the new ones only - didn't want to break bincompat by dropping existing ones. |
|
Not sure why removing the methods from |
kubukoz
added
‼️ binary breaking ‼️
‼️ usage breaking ‼️
|
yo github wtf, I just pushed a merge from series/0.17 into this 🤔 |
|
Looks like that did it... |
| @@ -231,8 +231,7 @@ lazy val scalacheck = projectMatrix | |||
| Dependencies.collectionsCompat.value, | |||
| Dependencies.Scalacheck.scalacheck.value | |||
| ), | |||
| libraryDependencies ++= munitDeps.value, | |||
| testFrameworks += new TestFramework("weaver.framework.CatsEffect") | |||
There was a problem hiding this comment.
This is set by the project plugin
| @@ -20,7 +20,7 @@ package internals | |||
|
|
|||
| import smithy4s.schema._ | |||
|
|
|||
| private[smithy4s] class ErrorCodeSchemaVisitor( | |||
| private[http] class ErrorCodeSchemaVisitor( | |||
There was a problem hiding this comment.
More narrow visibility scope here - let me know if that makes sense
|
alright pls review @Baccata <3 |
| @@ -53,6 +53,7 @@ class Smithy4sCli[Alg[_[_, _, _, _, _]], F[_]: MonadThrow]( | |||
| ): List[String] = | |||
| HttpEndpoint | |||
| .cast(endpoint) | |||
| .toOption | |||
There was a problem hiding this comment.
minor: since we're throwing a few lines below, would it be worth it to throw here also rather than just swallow the error?
There was a problem hiding this comment.
actually, I don't think so...
If HttpEndpoint.cast fails, that means the endpoint is not a HTTP one and this method should just return Nil. We're throwing iff HttpEndpoint.cast succeeds and the Http hint is missing, which would be a completely invalid state.
The purpose of protocolSpecificHelp is to generate a list of additional help strings based on... protocol-specific info 😅
so if you're implementing a CLI for a service that only exposes a CLI (and no HTTP interface), you want Nil here.
There was a problem hiding this comment.
I do the same in Playground and I've been thinking about it recently: kubukoz/smithy-playground#139 - basically, if protocol-specific interpreters are allowed to prefix URLs, maybe we shouldn't trust the Http hint. Or HttpEndpoint, for that matter. Not sure what to think about this, but it's a separate problem from this PR I think.
Closes #492.