Skip to content
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

Solve #164, make http.py return ordered headers. #188

Closed
wants to merge 2 commits into from

Conversation

sunhao2014
Copy link
Collaborator

Solve #164, make http.py return ordered headers, also add the related unit test and setup dependencies.

BTW, according to Kiran's suggestion, some invalid http header test cases were added.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 08e6144 on kbandla:http_ordered_header into 5b55332 on kbandla:master.

@sunhao2014
Copy link
Collaborator Author

Just noticed that the PR failed the auto build.
image
The reason why py26 didn't work is we used the collections.OrderedDict which is supported by py27. Thus how do we deal with this problem? Should we add support to Python versions older than 2.7?

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 36b81cd on kbandla:http_ordered_header into 5b55332 on kbandla:master.

@kbandla
Copy link
Owner

kbandla commented Jun 16, 2015

We could have, if ordereddict was a necessity. However:

  • many users probably dont need the exact header order
  • python2.6 is still widely used

How about doing this instead:
try to import collections. On ImportError, use a standard python dict. If not, use collections.OrderedDict

@brifordwylie
Copy link
Collaborator

We could pull https://pypi.python.org/pypi/ordereddict as a dependency. One of the nice things about dpkt is we have no requirements outside the standard library, seems a shame to add a dependency just for this small thing.

@brifordwylie
Copy link
Collaborator

Just saw Kiran's comment, yeah that's the right thing to do. Also +10 for automated CI :)

@@ -4,11 +4,12 @@

import cStringIO
import dpkt
import collections
Copy link
Owner

Choose a reason for hiding this comment

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

try:
from collections import OrderedDict
except ImportError:
OrderedDict = dict

@saylenty
Copy link
Contributor

Guys, is it really usefull feature? If we use ordered dict we would lose in speed of execution (and I think even in seconds). Once, I tried to optimise one problem (https://github.com/Saylenty/algorithm_with_dots-matrices) trying to solve it using ordered set, but this was not a good idea.

@obormot
Copy link
Collaborator

obormot commented Jun 16, 2015

@saylenty +1

@kbandla
Copy link
Owner

kbandla commented Jun 16, 2015

Well, it is true too. OrderedDict is written in python (vs C).

What if we do this instead? (This will not break any existing code either)

def parse_headers(f, ordered=False):
    d = {}
    if ordered:
        from collections import OrderedDict
        d = OrderedDict()
   ....

@obormot
Copy link
Collaborator

obormot commented Jun 16, 2015

@kbandla I like this option... but tbh my favorite solution is to not implement this at all :-) the HTTP protocol spec does not dictate any order in the headers, so dpkt is solid there. If someone wants them in order for their specific use case then it's easy to write their own parse function and feed it tcp.data.

@kbandla
Copy link
Owner

kbandla commented Jun 16, 2015

👍 @obormot. lets not implement this. go ahead and remove it @sunhao2014 (ordereddict support)

@sunhao2014
Copy link
Collaborator Author

Sure. I'll close this PR and send a new PR that only contains the added http invalid test cases.

@sunhao2014 sunhao2014 closed this Jun 17, 2015
@sunhao2014 sunhao2014 deleted the http_ordered_header branch June 17, 2015 01:22
@saylenty
Copy link
Contributor

@kbandla , Personally, I would rather implement this feature using OrderedDIct as you've written and write a warning only in the documentation that it can be much slower...

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

Successfully merging this pull request may close these issues.

None yet

7 participants