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

add handling for unset request types in endpoint #137

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

OsakiTsukiko
Copy link

No description provided.

@renerocksai
Copy link
Member

Please elaborate. There already exists nop() for unset handlers. So this patch adds a kind of "and set all the other ones to this function here" - which I do not like. I like it to be explicit. It's 7 fields to set in total.

@renerocksai
Copy link
Member

So, at the moment, I see it as a matter of taste. But if you can give me a compelling argument, I might reconsider "my taste" :-)

@OsakiTsukiko
Copy link
Author

oh, it's just for ease of use, i needed a way to set all unused handlers to a 404 handler and setting all the other fields to my handler didn't occur to me for some reason only after making this 😭 but yea, its just so you can easily set a function to catch all unset handlers so you don't forget one or something. I guess it would be more useful if there were no nop and it would force you to handle all requests so that you don't have hidden behavior, for me at first I was expecting the main listener to catch this instead of the endpoint and I did not understand why it was a valid request but I didn't get any output, if this makes any sense.

@renerocksai
Copy link
Member

renerocksai commented Oct 27, 2024

Yes, it makes total sense! You make a convincing consistency-argument here!

My suggestion:

  • we leave the nop() in there. But we make it a pub fn, so you can provide .unset = zap.Endpoint.nop at Endpoint.init.
  • the orelse chains could be modified like this:
    .get = s.get          // line-breaks just for GitHub comment
           orelse s.unset 
           orelse @compileError("Endpoint handler `.get` is unset, and no `.unset` handler is provided.")
    

WDYT? Maybe we should even rename nop() to nop_handler(), to make its name more descriptive.

@OsakiTsukiko
Copy link
Author

Sounds good. I'll get to changing it right away.

@OsakiTsukiko
Copy link
Author

added the requested changes, went for @panic instead of @compileError, the former not forcing comptime and changed nop to dummy_handler.

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.

2 participants