MatthieuDartiailh / bytecode

Python module to modify bytecode
https://bytecode.readthedocs.io/
MIT License
302 stars 38 forks source link

Add support for general constants through a modified key_const function #34

Closed MatthieuDartiailh closed 6 years ago

MatthieuDartiailh commented 6 years ago

Alternative to #33

@serhiy-storchaka @thautwarm

codecov-io commented 6 years ago

Codecov Report

Merging #34 into master will increase coverage by 0.06%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   93.58%   93.65%   +0.06%     
==========================================
  Files          17       17              
  Lines        2605     2617      +12     
==========================================
+ Hits         2438     2451      +13     
+ Misses        167      166       -1
Impacted Files Coverage Δ
bytecode/concrete.py 94.8% <100%> (-0.02%) :arrow_down:
bytecode/instr.py 93.9% <100%> (ø) :arrow_up:
bytecode/tests/test_concrete.py 100% <100%> (+0.26%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a7cc7a5...9db6154. Read the comment docs.

thautwarm commented 6 years ago

https://github.com/vstinner/bytecode/pull/34/files/bf0d8348ac79a1f1ad2ee24bcb634c96d6595e74#r211472739

If it doesn't break the working of related codes, why not remove the obj in const_key's return? Actually I used to think about giving an implementation(see https://github.com/vstinner/bytecode/issues/32#issuecomment-413532398) exactly as this PR, however I'm afraid of the prospective usage of the obj in const_key's return.

Could you please tell @serhiy-storchaka and me why we need an obj itself in its const_key?

thautwarm commented 6 years ago

Oh, I see. You've made the corresponding changes in related codes, great.

thautwarm commented 6 years ago

Could we merge this PR now? I'm looking forward to it :)

MatthieuDartiailh commented 6 years ago

Once Travis comes back happy I will merge yes. Sorry for the delay.