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

F601 - dictionary key repeated with different values #1441

Merged
merged 1 commit into from
Oct 3, 2018

Conversation

guedou
Copy link
Member

@guedou guedou commented May 21, 2018

Another PR of the #1277 serie. It removes duplicated dictionary keys.

@guedou guedou added the PEPin Apply PEP08 rules label May 21, 2018
@guedou guedou force-pushed the PEPin_F601 branch 3 times, most recently from 1afd529 to 5ce1fe7 Compare May 22, 2018 11:44
@codecov-io
Copy link

codecov-io commented May 22, 2018

Codecov Report

Merging #1441 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1441      +/-   ##
==========================================
+ Coverage   82.95%   82.96%   +<.01%     
==========================================
  Files         179      179              
  Lines       42230    42230              
==========================================
+ Hits        35032    35035       +3     
+ Misses       7198     7195       -3
Impacted Files Coverage Δ
scapy/contrib/homeplugav.py 92.47% <ø> (ø) ⬆️
scapy/layers/bluetooth.py 86.54% <ø> (ø) ⬆️
scapy/utils.py 75.07% <ø> (ø) ⬆️
scapy/sendrecv.py 80.57% <0%> (-0.65%) ⬇️
scapy/layers/inet6.py 87.4% <0%> (-0.06%) ⬇️
scapy/asn1/ber.py 82.38% <0%> (+0.28%) ⬆️
scapy/automaton.py 76.34% <0%> (+0.55%) ⬆️
scapy/layers/tls/record.py 92.12% <0%> (+0.87%) ⬆️

@p-l-
Copy link
Member

p-l- commented May 23, 2018

Why do you add noqa comments rather than removing (maybe commenting?) the duplicated keys which, anyway won't be used?

@guedou
Copy link
Member Author

guedou commented May 23, 2018 via email

@p-l-
Copy link
Member

p-l- commented May 23, 2018

The thing is when we leave two entries with the same key, we do not know which one will be chosen (this may change with different Python version). I think it would be cleaner to explicitly chose the value that will exist in the dictionary rather than letting the Python interpreter chose for us.

@guedou
Copy link
Member Author

guedou commented May 23, 2018

@p-l- I agree but I don't know which values should be kept =/

@guedou
Copy link
Member Author

guedou commented May 29, 2018

@p-l- what's your call on this PR?

@gpotter2
Copy link
Member

gpotter2 commented May 30, 2018

The real issue is that MIBDict use a weirdo-inverted format that makes no sense.

To properly fix this, we need to fix what args MIBDict takes, and revert all keys/values

@guedou
Copy link
Member Author

guedou commented Sep 8, 2018

@gpotter2 can you explain what could be done here?

@guedou guedou force-pushed the PEPin_F601 branch 2 times, most recently from b546a3d to d3263f0 Compare September 29, 2018 17:33
@guedou
Copy link
Member Author

guedou commented Sep 29, 2018

@gpotter2 @p-l- rebased and ready to be reviewed.

This is now as small PR.

@p-l- p-l- merged commit 0fd7d76 into secdev:master Oct 3, 2018
guedou pushed a commit to guedou/scapy-issues that referenced this pull request Oct 3, 2018
F601 - dictionary key repeated with different values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PEPin Apply PEP08 rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants