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

cmd/abigen: refactor command line interface #19797

Merged
merged 2 commits into from
Jul 8, 2019

Conversation

rjl493456442
Copy link
Member

This PR does three things:

  1. Refactor command line interface
  2. Introduce combined-json flag so that user can use json file directly
  3. ABI STDIN mode should read ABI only.

This PR is based on #19718.

@rjl493456442 rjl493456442 requested a review from gballet as a code owner July 5, 2019 14:51
@gballet
Copy link
Member

gballet commented Jul 5, 2019

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.

@rjl493456442 rjl493456442 force-pushed the abigen-support-mixins-2 branch from 39dc646 to 99e4988 Compare July 8, 2019 08:40
@rjl493456442
Copy link
Member Author

@gballet rebased. ptal

@rjl493456442
Copy link
Member Author

@gballet the only reason the use gopkg.in/urfave/cli.v1 to replace the flag is we can use utils.CheckExclusive method to ensure only one source is selected. So that we can simplify the checking logic a bit.

@gballet
Copy link
Member

gballet commented Jul 8, 2019

yeah, in any case it's consistent with the rest of the codebase, so it's a good change

Copy link
Member

@gballet gballet left a 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
Copy link
Member

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)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Tested, LGTM.

@gballet gballet added this to the 1.9.0 milestone Jul 8, 2019
@gballet gballet merged commit 2206061 into ethereum:master Jul 8, 2019
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 19, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 21, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 26, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Dec 28, 2024
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.

2 participants