Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 default headers to
WebSocket
s implementations #1606Add default headers to
WebSocket
s implementations #1606Changes from 46 commits
d8a87a5
170f16a
bd28f39
83a29e8
eabe380
5ecfd07
d954ef1
c7546bd
eef254d
f7fc772
87408cf
62c6c7e
4928484
d4c55c7
8e44b69
4c860f7
728b4b9
2beab89
7a8aa99
94ddfe5
49b135b
f4ada86
7a08ff0
137fd38
e241caa
eede8f4
62cbf96
877a9fc
1cbf30c
b061ede
02c96e4
3c6c915
9b79b31
1548e92
2b7267f
402be7b
85c342b
60f62c0
f290fe7
dc8a505
019e7d2
59cf6f4
866f3e9
f6a0872
e31cd53
9aa02c3
93af5ac
a86da4b
7daff74
97ca634
666a8b7
437c066
27bd66f
50d2e9f
37dcbd5
adddfc3
52d871b
0be0e7c
5673fa3
b51c8c2
c05759a
b19f58a
f03c77a
09bcd73
321c8e6
da1f87f
8b545ad
afae8ed
1768b31
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not 100% true. I forgot to mention... The
Date
headers from thewebsockets
cannot be removed... 🤔Should we do something about it on the tests? It's on purpose... 🤔
It's a question, I'm still thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do what on tests? There is no way to remove date header in
websockets
I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, I know, but the tests make us believe that we forgot about
Date
headers... 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to remove the
_added_names
logic?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me remove and check if it causes any issue. If it doesn't, I will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, removing the logic results in the following error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see if there is a way...I think its a limitation in Wesockets library. It is not accepting multiple headers with same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have raised an issue in Websockets repo to get a confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update:
As discussed here (python-websockets/websockets#1226 (comment)), it seems like its a bug to be fixed on Wesockets side.
@Kludex, Aymeric asked the use-case for having multiple server headers. I don't see any benefit as such, but is there a better answer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have PR python-websockets/websockets#1227 to fix the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR was closed as any fix to it was causing backward compatibility issues. 😔
Can we stick to
_added_names
logic?😟There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the other way is just check for
Server
headerThe advantage here is, users can still set other arbitrary headers with same names. Restrictions will be only on Server header.