nitrogen / simple_bridge

A simple, standardized interface library to Erlang HTTP Servers.
MIT License
112 stars 76 forks source link

Fix dialyzer errors. #63

Closed dhull closed 7 years ago

dhull commented 7 years ago

This pull request contains a number of small fixes for simple_bridge 2.0.1 revealed by dialyzer, in order of controversy.

The first two commits fix genuine bugs that dialyzer uncovered: an incorrect conversion, and a function call with the arguments in the wrong order.

The third commit converts a now() call to os:timestamp(). Dialyzer warns that now is deprecated, and the os:timestamp call is equivalent (and faster).

The fourth commit removes a case clause that dialyzer says is unnecessary. I think dialyzer is correct, but even if it isn't the only difference is a slightly different error is returned from simple_bridge:inner_make/2.

The fifth commit silences another dialyzer warning, where is says that a function head is never matched. I believe that dialyzer is right, but rather than removing the code, I added a call from a dummy function to silence the warning.

Feel free to take as many or as few of these commits as you want, and to modify them as you see fit.

choptastic commented 7 years ago

Thanks David,

These look good.

I suspect the timer is a remnant of when I was first building the websocket hijacker. There's definitely no reason to keep it there.

You also may be right about the apply_mask dialyzer warning. I honestly can't say why I added that specific header clause. It's been so long since I wrote it, and I didn't comment on it like I should have. I'll try taking it out and see what happens with the websocket tests.

My gut tells me that when I was building it (and timing it), I was probably passing in binary strings for testing if it worked, and using that clause to convert the binaries to integer, so it's likely also a remnant from testing.

I'll give this a few tests and make sure it's all good, but I'm pretty confident this is fine :)

Thanks for the PR!

choptastic commented 7 years ago

Merged. Thanks!

choptastic commented 7 years ago

Oh, and by the way: I removed the apply_mask header after testing that its removal was unaffected by the autobahn test suite, and I removed the timer.