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

Deprecate HttpApp and remove internal usage. #2791

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

987Nabil
Copy link
Contributor

@987Nabil 987Nabil commented Apr 21, 2024

It seems that Routes and HttpApp are at the end just two wrappers aroud a set of routes. Therefore this PR tries to simplify the structure

@987Nabil 987Nabil marked this pull request as ready for review April 21, 2024 20:31
@987Nabil 987Nabil requested review from jdegoes and vigoo as code owners April 21, 2024 20:31
@987Nabil 987Nabil changed the title Provide HandlerAspect context via env instead of function param (#2488) Deprecate Routes and remove internal usage. Apr 22, 2024
@@ -232,9 +233,11 @@ val intAspect: HandlerAspect[Any, Int] = HandlerAspect.identity.as(42)
To access this integer value in the handler, we need to define a handler that receives a tuple of `(Int, Request)`:

Copy link
Member

Choose a reason for hiding this comment

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

The description of the snippet is required to be refactored.

@@ -258,9 +261,10 @@ val intStringAspect: HandlerAspect[Any, (Int, String)] =
Correspondingly, to access the output context of this `HandlerAspect`, we need to have a handler that receives a tuple of `(Int, String, Request)`:

Copy link
Member

Choose a reason for hiding this comment

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

Also this.

@jdegoes
Copy link
Member

jdegoes commented Apr 22, 2024

@987Nabil I noticed you kept HttpApp and removed (deprecated) Routes. I would do it the other way around: HttpApp is less standard terminology than a pluralization of Route.

@987Nabil
Copy link
Contributor Author

@jdegoes I'll switch it around

@987Nabil 987Nabil changed the title Deprecate Routes and remove internal usage. Deprecate HttpApp and remove internal usage. Apr 23, 2024
@987Nabil 987Nabil force-pushed the eliminate-routes branch 2 times, most recently from a6e9282 to aac2912 Compare April 23, 2024 16:41
@@ -195,34 +195,18 @@ trait Routes[-Env, +Err] {
}
```

## Converting `Routes` to `HttpApp`

Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing this section, I think we can refactor it and move it to the serving section to explain why we need handled routes to serve them.

@jdegoes
Copy link
Member

jdegoes commented Apr 23, 2024

@987Nabil Let me know when build is passing so we can get this in sooner rather than later.

@987Nabil 987Nabil force-pushed the eliminate-routes branch 5 times, most recently from b8c3b92 to 6855895 Compare April 24, 2024 11:51
@987Nabil
Copy link
Contributor Author

@jdegoes build is green

@jdegoes jdegoes merged commit 5312306 into zio:main Apr 24, 2024
25 checks passed
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.

3 participants