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

WIP: Use ctx when elli_ctx is set #44

Closed
wants to merge 1 commit into from

Conversation

yurrriq
Copy link
Member

@yurrriq yurrriq commented Mar 6, 2018

@tsloughter, feel free to implement where I've got throw(nyi). I think I got them all... I figure we could maybe just fold in your ctx module as elli_ctx, unless you want it to stay generic.

{erl_opts, [{d, elli_ctx}]}.

^ this will trigger a bunch of failures as of now

@yurrriq yurrriq mentioned this pull request Mar 6, 2018
@coveralls
Copy link

coveralls commented Mar 13, 2018

Coverage Status

Coverage increased (+0.4%) to 76.486% when pulling 17cbcc8 on feature/abstract-elli_req into f56f2bc on develop.

This way we can easily conditionally use ctx.
@tsloughter
Copy link
Member

Hey, sorry, looking at this again now. It seems to require putting stuff like the socket in the context?

I think we'd need to instead move to a 3 arity handle function: handle(Ctx, Req, Args)

@yurrriq
Copy link
Member Author

yurrriq commented May 5, 2018

Yeah, it does look that way.. I can't remember what I was thinking here. I do like the further adoption of #req{} accessor functions though. Seems like it'd make it easier to transition away from using a record, if we decide to use maps in the future.

I wonder if we couldn't just implement an elli_ctx_middleware module, and check handlers for handle/3 (pass context) or handle/2 (behave like elli_middleware).

I'll pull out the req accessors parts into a new PR, and can draft an elli_ctx_middleware today or tomorrow, if you like.

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