Make type Scheme follow the specs #2490
Conversation
FabioPinheiro
requested review from
amitksingh1490,
jdegoes,
vigoo and
adamgfraser
as code owners
|
|
|
Is there benchmark that I can run here? |
|
|
||
| /** | ||
| * Decodes a string to an Option of Scheme. Returns None in case of | ||
| * null/non-valid Scheme | ||
| * | ||
| * Scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) |
There was a problem hiding this comment.
I was thinking about add a regex for this.
Also make sure everything is lowercase.
But I was concerned about efficient. Should we create a new method for that
There was a problem hiding this comment.
I am not sure if we should check the validity here. But I think we should ignore casing
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2490 +/- ##
==========================================
+ Coverage 64.95% 65.01% +0.05%
==========================================
Files 136 136
Lines 7197 7208 +11
Branches 1296 1300 +4
==========================================
+ Hits 4675 4686 +11
Misses 2522 2522
☔ View full report in Codecov by Sentry. |
|
|
||
| /** | ||
| * Decodes a string to an Option of Scheme. Returns None in case of | ||
| * null/non-valid Scheme | ||
| * | ||
| * Scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) |
There was a problem hiding this comment.
I am not sure if we should check the validity here. But I think we should ignore casing
FabioPinheiro
force-pushed
the
fix/issue2489-Make_type_Scheme_following_the_specs
branch
from
70b1124
to
230baf1
Compare
FabioPinheiro
force-pushed
the
fix/issue2489-Make_type_Scheme_following_the_specs
branch
from
c9151cc
to
2b46f7a
Compare
Add environment variable (USE_SNAPSHOTS=true) to control the build Update zio-http to 3.0.0-RC3-SNAPSHOT (Fix for CORS) zio-http with CORS fix when origin: chrome-extension See zio/zio-http#2490
| @@ -78,4 +89,15 @@ object Scheme { | |||
| case object WS extends Scheme | |||
|
|
|||
| case object WSS extends Scheme | |||
|
|
|||
| /** | |||
| * @param scheme | |||
There was a problem hiding this comment.
What would actually happen, if this is http(s)/ws(s)?
There was a problem hiding this comment.
It's just a question of pattern matching.
We are allowing two representations for the same value. (Scheme.HTTP and Scheme.Custom("http"))
Since the construction method is public.
But I see this pattern is being used everywhere in zio-http so it's a more general problem
One solution would be to make it private and the only way to create Custom is with the Scheme's method def decode(scheme: String).
This would make it a bit more type-safe. Instead of being on the documentation
Make type Scheme more flexible
Fix for #2489 Follow the specs:
I'm just concerned about efficiency and keep the source code interface as similar as possible.