-
Notifications
You must be signed in to change notification settings - Fork 167
refactor(fill,deps): forbid yul code in python tests; remove solc-select
dependency
#1779
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
Conversation
b4a1242
to
c31efcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this @felix314159!
Just a general remark, I prefer smaller PRs and would have preferred if the test refactor from solc to Opcodes
was a separate PR. In this case the task list in the issue could have guided you on this path. But we all do it (I'm guilty of it in #1765; I should have done a preliminary refactor PR first).
I think I wasn't aware that the static tests require solc
(it's obvious in hindsight, but I thought they only required lllc
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups, was trigger happy with the "Submit Review" button and submitted before I was finished. Here's the rest of the review :)
Docs also need to remove all references to solc-select/yul (installation, installation troubleshooting, contributing?).
Context on why a coverage fixer test was added: by removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Felix, thanks for improving this! I ran out of time before I managed to complete a comprehensive review this morning. But here's some intermediate feedback, will prioritize this on Monday!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @felix314159, continuing on from the reviews above, here's a few more comments. Thanks!
529b1da
to
1e46ecf
Compare
seems like i messed up this rebase, ill try to revert |
1e46ecf
to
afe9adf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your huge effort on this. And your willingness to go the extra mile to make this PR atomic with updated docs.
One comment on test_coverage_script_fix
: I think we can remove this function safely, if you agree, please do that and then we can get this merged!
Note: when i run Edit: Next commit fixes this |
a3b7ea1
to
ea9ac45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @felix314159 LGTM!
solc-select
dependency
solc-select
dependencysolc-select
dependency
ποΈ Description
See issue #1759.
π Related Issues
β Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.