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 ?

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

Successfully merging this pull request may close these issues.

2 participants