-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
cmd/abigen: refactor command line interface #19797
Conversation
I see that some of the changes from my PR are included in there. So it's dependent 😓 Ok, I'll be as quick as possible. |
39dc646
to
99e4988
Compare
@gballet rebased. ptal |
@gballet the only reason the use |
yeah, in any case it's consistent with the rest of the codebase, so it's a good change |
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 overall, please address the two questions that I have. I'm not finished testing yet, I'm just giving you a head start while I complete the testing.
} | ||
var lang bind.Lang | ||
switch *langFlag { | ||
switch c.GlobalString(langFlag.Name) { | ||
case "go": | ||
lang = bind.LangGo | ||
case "java": | ||
lang = bind.LangJava | ||
case "objc": | ||
lang = bind.LangObjC |
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.
I know I've been telling you to reactivate LangObjC
because Peter rejected my PR to remove it (it's alway's Peter's fault), this being said we should put a warning here that ObjC support is incomplete.
} | ||
var userdoc, devdoc interface{} | ||
json.Unmarshal([]byte(info.Userdoc), &userdoc) | ||
json.Unmarshal([]byte(info.Devdoc), &devdoc) |
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.
What's the reasoning behind no longer checking the error here? UserDoc
indeed doesn't seem to be used over the codebase, and if that it the case then let's remove it (not preferred) or report if something is invalid.
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.
Because user can select which information should be included in the combined-json
.
See the CLI description: --combined-json abi,asm,ast,bin,bin-runtime,compact-format,devdoc,hashes,interface,metadata,opcodes,srcmap,srcmap-runtime,userdoc
Since Userdoc
and Devdoc
are not necessary for binding generation, so that error shouldn't be returned for the field absense.
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.
Tested, LGTM.
This PR does three things:
combined-json
flag so that user can use json file directlyABI
STDIN mode should read ABI only.This PR is based on #19718.