Made websocket subprotocols conform to spec

[flying-sheep]

fixes #178

[asvetlov]

Hmm.
I doubt that protocol mismatch should produce UserWarning.
But we definitely need some indication.
Maybe just protocol is None check for do_handshake() return value is enough?

[flying-sheep]

why do you think a warning is unsuited here?

something wants to connect with a protocol that isn’t supported: that’s not normal, and a log entry might help debugging instead of silently succeeding with a return value that might not be checked.

[asvetlov]

Warnings mean something's wrong with your code that won't get better by ignoring it, and you should fix it at some point.

I see nothing wrong with my code if client requests non-supported protocol. Server code should not changed to "fix" warning, problem is inside incompatible client.
The issue may be solved by proper logging, not warnings.warn().

[ludovic-gasc]

I'm agree with @asvetlov
For me, you can use warnings python module when you make a Python library and you want to alert developers if you break the API for the next version.

In this case, I strongly vote to use a logger with warning level.
For the daemons I made, I use this logging configuration: https://github.com/Eyepea/API-Hour/blob/master/examples/project_template/%7B%7Bcookiecutter.app_name%7D%7D/etc/%7B%7Bcookiecutter.app_name%7D%7D/logging.ini

If you use warnings instead of logging for that, I don't receive e-mail that something is wrong with a client.

[flying-sheep]

it’s up to him then to introduce logging, and this will wait?

[asvetlov]

it’s up to him then to introduce logging, and this will wait?

@flying-sheep sorry, I don't follow.

[flying-sheep]

I meant: this project doesn’t use any logging yet AFAICT. So you should introduce logging how you see fit (central logger with just basicConfig() or per-module logger, maybe even finer grained. using name, hardcoded names or custom logger classes, yadda yadda)

and after i see how you want logging to be done, i’ll update this pull request to use logging.

[ludovic-gasc]

Do you talk about this: https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/log.py ?
If yes, you could add a logger for websockets.

[asvetlov]

Yes, I think ./aiohttp/log.py is good place for new logger. You can call it "aiohttp.websocket" for example.

[flying-sheep]

it appears that i am blind or looked at an old version or something.

in any case, i didn’t know of the existence of log.py in this project, so it’s clear why you’re confused by my wrong-assumption-fueled comments -.-

[flying-sheep]

done!

[asvetlov]

Thanks!

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.