-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Support target artifact selection in JSON I/O #2146
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
3561ab1 to
06903c9
Compare
| bool isTargetRequired(Json::Value const& _targets, string const& _target) | ||
| { | ||
| for (auto const& target: _targets) | ||
| if (target == _target) |
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.
Need to support sub matches, e.g. evm.assembly should be enabled by both evm.assembly, evm, evm.*, etc.
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.
@chriseth do you have any code suggestion for doing so? E.g. requiring evm.assembly matches both the user supplied evm.assembly, evm.* and evm, without hardcoding these paths.
5255f71 to
bdf2ddd
Compare
bdf2ddd to
f214c66
Compare
f214c66 to
a2fc9b1
Compare
a2fc9b1 to
27ab6bf
Compare
|
What is left to do here? Can you work on this next, @axic? |
|
Reviewed it again, I think the API ( It also doesn't support wildcards properly and shortest match. |
27ab6bf to
fa6f8b3
Compare
5027c83 to
a7aa3bc
Compare
|
Need to add tests for matching different combinations ( |
| }, | ||
| "outputSelection": { | ||
| "fileA": { | ||
| "A": [ "abi", "devdoc", "userdoc", "evm", "metadata" ] |
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.
This is annoying, one of the reasons for the failing tests was a missing comma here at the end 😕
d8ac20f to
1e62516
Compare
a6a62b6 to
dea2b70
Compare
378c0d5 to
c369352
Compare
Changelog.md
Outdated
| Features: | ||
| * Parser: Better error message for unexpected trailing comma in parameter lists. | ||
| * Standard JSON: Support the ``outputSelection`` field for selective compilation of supplied sources. | ||
| * Standard JSON: Support the ``outputSelection`` field for selective compilation. |
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.
Please move to 0.4.19
bd622d1 to
64bd962
Compare
|
Depends on argotorg/solc-js#159. |
64bd962 to
76b9165
Compare
|
@chriseth This is working now (as confirmed by circleci). It still needs support for shorcuts (e.g. |
|
@chriseth can you review this? it may be worth even merging it in this state, though more tests cannot hurt. |
| input["settings"]["optimizer"]["runs"] = 200; | ||
|
|
||
| // Enable all SourceUnit-level outputs. | ||
| input["settings"]["outputSelection"]["*"][""][0] = "*"; |
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.
Does [0] automatically make it an array?
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.
Yep. Had an ugly large code before to do it manually.
| return sources; | ||
| } | ||
|
|
||
| bool isTargetRequired(Json::Value const& _targets, string const& _target) |
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.
Perhaps better requested than required?
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.
Sure, but also please see my other naming suggestion below.
| { | ||
| /// Special case for SourceUnit-level targets (such as AST) | ||
| if ( | ||
| _targets[file].isMember("") && |
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.
Can you extract this condition into a function and also use it in line 129?
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.
129?
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.
Sorry, 179
c9f237f to
cb9fe1d
Compare
|
@chriseth I think this should be ready now? |
cb9fe1d to
8a86249
Compare
8a86249 to
9b9405e
Compare
|
@chriseth added two more commits to simplify/optimize it even more (last 3 commits should be squashed). Please check those two. |
|
Looks good! |
9b9405e to
cb06f02
Compare
cb06f02 to
3576ccf
Compare
Fixes #2897. Depends on #2729.