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 Plug.Conn.Status.valid?/1 #1156

Open
halostatue opened this issue Jul 17, 2023 · 2 comments
Open

Add Plug.Conn.Status.valid?/1 #1156

halostatue opened this issue Jul 17, 2023 · 2 comments

Comments

@halostatue
Copy link
Contributor

halostatue commented Jul 17, 2023

I’m working on a library that accepts status codes (integer or atom) and want to verify that valid status values are used.

I can do this with:

try
  Plug.Conn.Status.code(value)
  true
rescue
  _ -> false
end

But this seems like it might be more generally useful if built into Plug.Status. Would a PR for this feature be accepted?

As well, it seems that Plug.Conn.Status.code/1 for the integer case isn’t entirely correct given the error raised with an unknown value in reason_phrase/1:

  def code(integer) when integer in 100..999 do
    integer
  end
…

That feels like it should be instead

```elixir
@valid_status_codes Map.keys(Map.merge(statuses, custom_statuses))
def code(integer) when integer in @valid_status_codes do
  integer
end
@halostatue halostatue changed the title Proposal: Status.valid?/1 Proposal: Plug.Conn.Status.valid?/1 Jul 17, 2023
@whatyouhide
Copy link
Member

The problem with HTTP status codes is that they are standardized, but not enforced. That means that a status code of 499 can be custom to an application using it, and still be perfectly valid HTTP. So, I’m not sure that valid?/1 here would make sense 🤔

@whatyouhide whatyouhide changed the title Proposal: Plug.Conn.Status.valid?/1 Add Plug.Conn.Status.valid?/1 Jul 18, 2023
@halostatue
Copy link
Contributor Author

The name could be changed (known?/1, well_known?/1, defined?/1), but I was basing the name valid?/1 on the exceptions thrown for reason_phrase/1 and reason_atom/1 with unknown values and the fact that Plug.Conn.Status is compile-time extensible (which, before looking at this, I didn’t realize).

From my perspective, it's more important that something like valid?/1 be usable for the atom versions to reduce the need for runtime exception handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants