Skip to content
This repository was archived by the owner on Jan 13, 2021. It is now read-only.

Enable override default headers from CLI #205

Merged
merged 3 commits into from
Feb 22, 2016
Merged

Enable override default headers from CLI #205

merged 3 commits into from
Feb 22, 2016

Conversation

pkrolikowski
Copy link
Contributor

@Lukasa I had a problem with push to rebased branch, so I decided to make a new request.

headers[i.key] = i.value
# :key:value case
if i.key == '':
k, v = i.value.split(':')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I don't think it's likely we could get a header with a colon in it (the only possible one is :path, which might be able to have a colon in it), we should defend against that anyway. Let's swap this to i.value.split(':', 1).

@Lukasa
Copy link
Member

Lukasa commented Feb 22, 2016

Fantastic, everything is green! I have a few minor style notes that I'd like you to change, if possible, but when that's done I think this will be ready to merge.

@pkrolikowski
Copy link
Contributor Author

I changed this block as you suggested.

else:
# when overriding a HTTP/2 special header there will be a leading
# colon, which tricks the command line parser into thinking
# the header is empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind indenting this one more level?

@Lukasa
Copy link
Member

Lukasa commented Feb 22, 2016

Sorry, I wasn't quite clear enough on the indentation. Fix that up and we're all good! =)

@Lukasa
Copy link
Member

Lukasa commented Feb 22, 2016

Magnificent, thank you so much @pkrolikowski! ✨ 🍰 ✨

Lukasa added a commit that referenced this pull request Feb 22, 2016
Enable override default headers from CLI
@Lukasa Lukasa merged commit 7aa2201 into python-hyper:development Feb 22, 2016
@pkrolikowski
Copy link
Contributor Author

I am glad I could help :) So what do you think, what could I do next ?

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

Successfully merging this pull request may close these issues.

2 participants