I'm unsure that you need to keep this code organization with added rescue block.

Why not something like

  begin
    HTTP.parse_headers_and_body(io) do |headers, body|
      new method, resource, headers, body, http_version
    end
  rescue ex : ArgumentError
    BadRequest.new
  end

@akzhan that doesn't work, there's a case where parse_headers_and_body returns without yielding which isnt handled.

I'm not entirely sure if this is the correct way to detect this. It feels to broad. I'm not sure what would be a better approach though.

@RX14 To clarify: are you thinking about more exceptions which could be thrown and would be handled like this?

Perhaps non-ArgumentError exceptions would be thrown, perhaps new would throw an exception. This PR is a great improvement but perhaps we could implement it a better way.

The best way probably would be not rescuing the exception but handling the case that triggers it explicitly/differently.

@jhass @RX14 updated, now it is handled explicitly in parse_headers_and_body while adding header. Ideas?

Ideally no exception should be raised nor rescued. We should maybe have a method to add a header and return false or nil if it's not valid, instead of raising.

Also some tests would be awesome :)

I'd suggest headers.add(name, value) if headers.valid_value?(value)

This checks the validity twice. I suggest adding add? method that returns false if it's not valid, true otherwise.

This allocates an array. The same for add(key, String). I prefer it not to allocate (like before, have different implementations of the method)

Or maybe we can just allow to pass a tuple here?

@asterite updated.
Any other suggestions?