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

fix: unable to import PyEVMBackend from fresh upgrade #213

Merged
merged 10 commits into from
Nov 18, 2021

Conversation

antazoey
Copy link
Contributor

@antazoey antazoey commented Nov 4, 2021

What was wrong?

fixes: #212

How was it fixed?

Check version when checking if py-evm installed

To-Do:

Cute Animal Picture

baby_squirrel

@antazoey antazoey changed the title Fix/unable to import fix: unable to import PyEVMBackend from fresh upgrade Nov 4, 2021
Copy link
Contributor

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Thanks for the change, you're so fast! This is sort of a nit, but the error messaging where is_pyevm_available gets called assumes that pyevm isn't installed (like here, and here), so I wonder if it would be better to make these two separate checks? Like leave is_pyevm_available, and then add a method that checks that the version of py-evm is a high enough version so we can make the error messages slightly better. Or we can update the wording of the error messages. What do you think?

@antazoey
Copy link
Contributor Author

antazoey commented Nov 4, 2021

here

I think what I would do then is change to the error messages to indicate pyevm >= 5.0.0 is not installed and maybe change the method name to is_supported_pyevm_version_available(). I think that would be easier because we will always have to check both anyway.


Edit: I just pushed those changes but will think on this overnight as well!

@kclowes
Copy link
Contributor

kclowes commented Nov 10, 2021

@unparalleled-js - This is ready to go, right?

@antazoey
Copy link
Contributor Author

Yeah, unless you know a way to dynamically get the min supported version. I tried to see if we could do that but couldn't find a way.

@kclowes
Copy link
Contributor

kclowes commented Nov 10, 2021

For this line/error messages? Is it set somewhere else?

@kclowes
Copy link
Contributor

kclowes commented Nov 12, 2021

I felt bad for breaking your CHANGELOG again and again, so I added towncrier to eth-tester. So now there will be newsfragments like our other repos, and less merge conflicts 🎉. You can pull in master to get that update. I'm happy to add the snippet from the CHANGELOG in this PR to a newsfragment if you want, just let me know!

@antazoey
Copy link
Contributor Author

I felt bad for breaking your CHANGELOG again and again, so I added towncrier to eth-tester. So now there will be newsfragments like our other repos, and less merge conflicts 🎉. You can pull in master to get that update. I'm happy to add the snippet from the CHANGELOG in this PR to a newsfragment if you want, just let me know!

I think I upgraded to the newsfragments approach correctly!

@kclowes kclowes merged commit 0d03374 into ethereum:master Nov 18, 2021
@antazoey antazoey deleted the fix/unable-to-import branch February 2, 2022 17:45
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.

Import error when installing update 0.6.0b1
2 participants