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

Better command line in rlp #4639

Merged
merged 13 commits into from Dec 7, 2017
Merged

Better command line in rlp #4639

merged 13 commits into from Dec 7, 2017

Conversation

@demon1999
Copy link
Contributor

demon1999 commented Oct 31, 2017

No description provided.

Copy link
Collaborator

chfast left a comment

Sorry for delays in reviews. Everyone is on devcon3.

else if (inputFile.empty())
inputFile = arg;
else
otherInputs.push_back(arg);
}
if (vm.count("help")) {
cout << "Usage rlp <mode> [OPTIONS]" << endl << modesOptions

This comment has been minimized.

Copy link
@chfast

chfast Nov 2, 2017

Collaborator

I'm not expert here, but shouldn't the program_options be able to output that for us?

This comment has been minimized.

Copy link
@demon1999

demon1999 Nov 3, 2017

Author Contributor

fixed

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 3, 2017

Codecov Report

Merging #4639 into develop will increase coverage by 0.18%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4639      +/-   ##
===========================================
+ Coverage     60.9%   61.09%   +0.18%     
===========================================
  Files          343      343              
  Lines        26999    27089      +90     
  Branches      2765     2779      +14     
===========================================
+ Hits         16444    16550     +106     
+ Misses        9603     9569      -34     
- Partials       952      970      +18
Impacted Files Coverage Δ
rlp/main.cpp 0% <0%> (ø) ⬆️
libethashseal/EthashProofOfWork.h 80% <0%> (-20%) ⬇️
libethereum/Transaction.h 53.84% <0%> (-18.38%) ⬇️
libethereum/Executive.h 52.17% <0%> (-14.5%) ⬇️
libethereum/Account.h 80% <0%> (-14.12%) ⬇️
libethcore/LogEntry.h 44% <0%> (-13.9%) ⬇️
libdevcrypto/Common.h 73.33% <0%> (-12.39%) ⬇️
libevm/VM.h 58.82% <0%> (-9.93%) ⬇️
libdevcore/CommonJS.h 90.32% <0%> (-9.68%) ⬇️
utils/json_spirit/json_spirit_value.h 57.52% <0%> (-9.15%) ⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf4e7c...7072fd4. Read the comment docs.

@gumb0 gumb0 self-requested a review Nov 23, 2017
@gumb0 gumb0 force-pushed the demon1999:developrlp branch from 1e3c084 to d7ff6fd Nov 28, 2017
Copy link
Member

gumb0 left a comment

Ok, the help output looks a little weird now:

Usage rlp <mode> [OPTIONS]
Modes:
  -i [ --indent ] arg   <string>  Use string as the level indentation (default
                        '  ').
  --hex-ints            Render integers in hex.
  --string-ints         Render integers in the same way as strings.
  --ascii-strings       Render data as C-style strings or hex depending on
                        content being ASCII.
  --force-string        Force all data to be rendered as C-style strings.
  --force-escape        When rendering as C-style strings, force all characters
                        to be escaped.
  --force-hex           Force all data to be rendered as raw hex.
  -D [ --dapp ]         Dapp-building mode; equivalent to --encrypt --64.
    create <json>  Given a simplified JSON string, output the RLP.
    render [ <file> | -- ]  Render the given RLP. Options:
    list [ <file> | -- ]  List the items in the RLP list by hash and size.
    extract [ <file> | -- ]  Extract all items in the RLP list, named by hash.
    assemble [ <manifest> | <base path> ] <file> ...  Given a manifest & files, output the RLP.
General options:
  -e [ --encrypt ]      Encrypt the RLP data prior to output.
  -L [ --lenience ]     Try not to bomb out early if possible.
  -x [ --hex ]          Treat input RLP as hex encoded data.
  --base-16             Treat input RLP as hex encoded data.
  -k [ --keccak ]       Output Keccak-256 hash only.
  --base-64             Treat input RLP as base-64 encoded data.
  --64                  Treat input RLP as base-64 encoded data.
  -b [ --bin ]          Treat input RLP as raw binary data.
  --base-256            Treat input RLP as raw binary data.
  -q [ --quiet ]        Don't place additional information on stderr.
  -h [ --help ]         Print this help message and exit.
  -V [ --version ]      Show the version and exit.

We should figure out how to make it better, modes (create, render, list, extract, assemble) should be on the top, after Modes:
The options on top I think make sense for the render mostly, so maybe they should have title "Render Options"

Also please move -D into General options, because it's a combination of two other general options, I think it fits there better

prefs.escapeAll = true;
if (vm.count("nice"))
prefs.forceString = true, prefs.stringInts = false, prefs.forceHex = false, prefs.indent = " ";
if (vm.count("lenience"))

This comment has been minimized.

Copy link
@gumb0

gumb0 Nov 28, 2017

Member

Here's the duplicated piece of code from lenience till encrypt

@gumb0

This comment has been minimized.

Copy link
Member

gumb0 commented Nov 28, 2017

What if we change it to rlp --mode=<mode>, would it be easier to make help output look better and could we then get rid of handling unrecognized options?

The challenge might be in different modes having different number of arguments following them (like assemble has arbitrary number of files)

Check out also "positional options". maybe it helps somehow http://www.boost.org/doc/libs/1_58_0/doc/html/program_options/tutorial.html#idp337627504

@gumb0

This comment has been minimized.

Copy link
Member

gumb0 commented Nov 29, 2017

What happened to the indentation? Looks like it all changed to spaces

@demon1999

This comment has been minimized.

Copy link
Contributor Author

demon1999 commented Nov 29, 2017

It's my fault. I'll fix it

Copy link
Member

gumb0 left a comment

Also further please use more descriptive commit messages than "Update main.cpp"

case Encoding::Keccak:
cout << sha3(_out).hex() << endl;
break;
case Encoding::Hex: case Encoding::Auto:

This comment has been minimized.

Copy link
@gumb0

gumb0 Nov 29, 2017

Member

Don't change the indentation of casess inside switches, I just fixed it before in my commit here

@@ -1,16 +1,13 @@
/*
This file is part of cpp-ethereum.

This comment has been minimized.

Copy link
@gumb0

gumb0 Nov 29, 2017

Member

Don't remove these empty lines in comments

@gumb0
gumb0 approved these changes Nov 30, 2017
@@ -98,8 +98,8 @@ include(EthExecutableHelper)
include(EthDependencies)
include(EthUtils)

hunter_add_package(Boost COMPONENTS filesystem system thread)
find_package(Boost CONFIG REQUIRED filesystem system thread)
hunter_add_package(Boost COMPONENTS program_options filesystem system thread)

This comment has been minimized.

Copy link
@chfast

chfast Dec 1, 2017

Collaborator

Hasn't this been merged to develop already?

<< renderOptions << generalOptions;
exit(0);
}
if (vm.count("lenience"))

This comment has been minimized.

Copy link
@chfast

chfast Dec 1, 2017

Collaborator

This seems to be a weird way of doing this. Can't we inform boost we would like to store this option in bool lenience variable and use the variable directly?

This comment has been minimized.

Copy link
@gumb0

gumb0 Dec 6, 2017

Member

This is the way suggested by the official tutorials http://www.boost.org/doc/libs/1_65_1/doc/html/program_options/tutorial.html#idp437555840
But there's an option to store it in a local variable, too, I think.

This way is actually more compact (no need to define a local variable for each option), but duplicating the option name is a downside

@chfast

This comment has been minimized.

Copy link
Collaborator

chfast commented Dec 6, 2017

Rebase and merge?

@gumb0

This comment has been minimized.

Copy link
Member

gumb0 commented Dec 6, 2017

I'll rebase it

demon1999 and others added 13 commits Oct 28, 2017
fixing rlp
fixing formating
…ort them
…ing.
@gumb0 gumb0 force-pushed the demon1999:developrlp branch from bd5cf49 to 7072fd4 Dec 6, 2017
@gumb0 gumb0 merged commit 40c7ceb into ethereum:develop Dec 7, 2017
7 of 9 checks passed
7 of 9 checks passed
codecov/patch 0% of diff hit (target 50%)
Details
codecov/project/tests 79.2% (-0.01%) compared to 2cf4e7c
Details
ci/circleci: Linux-Clang5 Your tests passed on CircleCI!
Details
ci/circleci: Linux-GCC6-Debug Your tests passed on CircleCI!
Details
ci/circleci: macOS-XCode9 Your tests passed on CircleCI!
Details
codecov/project 61.09% (+0.18%) compared to 2cf4e7c
Details
codecov/project/app 53.07% (+0.3%) compared to 2cf4e7c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.