search

nucleic / enaml

Declarative User Interfaces for Python
http://enaml.readthedocs.io/en/latest/
Other
1.53k stars 130 forks source link

Fix python3.9 support #443

Closed pbehnke closed 3 years ago

pbehnke commented 3 years ago

python3.9 was broken due to missing import.

MatthieuDartiailh commented 3 years ago

Thanks for your contribution. I took the liberty to use the singleton Load defined in base_parser rather creating new instances, and added some comments since I could not remember the reason behind those changes. If you can check It does fix your issue it would be great.

codecov-io commented 3 years ago

Codecov Report

Merging #443 (5a60fd3) into main (23a5633) will decrease coverage by 2.94%. The diff coverage is 42.85%.

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
- Coverage   73.44%   70.50%   -2.95%     
==========================================
  Files         313      313              
  Lines       23976    23976              
==========================================
- Hits        17609    16904     -705     
- Misses       6367     7072     +705
pbehnke commented 3 years ago

Hello,

Yes, I noticed the Load class in base_parser and initially tried that approach (https://github.com/nucleic/enaml/pull/443/commits/48c4b4748ab2d60b3dec69fb56896633fe28c29c), but the Load object defined there is an instance rather than a class and therefore is not callable. You were already calling Load() here, so went with importing it from the ast module since it seemed to have a minimal impact on the rest of the code. I've tested this with enaml-web and it seems to fix the issue, which was just this missing symbol (Load) when using python3.9. Would you prefer passing the same instance to each of the ast.Tuple() calls? I haven't tried this as it seemed inconstant with the original intention, but can try it out if you'd like.

Great work on this project, btw. I'm love the ideas you've presented here; the use of kiwisolver is brilliant.

MatthieuDartiailh commented 3 years ago

I already made the change to use the singleton from base_parser but had not time to add tests, so if you could test on your end (or better write test) it would be great.

pbehnke commented 3 years ago

Ah! My bad; I missed seeing your commit. I rebuilt with this new change and it appears to have fixed the issue! Thank you!

MatthieuDartiailh commented 3 years ago

Thanks for checking ! I will merge then