Command line parser issues

[movitto]

This pull request addresses issue #2983 with the following changes:

• Expand the signatures of cli commands corresponding to parseAccountItems and parseAccountCurrencies to accept a 3rd 'strict' parameter
• Remove the edge case omitting output in the case of bad syntax, now output is always produced

As far as the 2nd bullet point in issue #2983, the aforementioned commands were inspected and found to output documentation pertaining to the 'strict' parameter as expected (see attached screenshot). If I missed something please let me know and I can look into further.

[nbougalis]

LGTM 👍

[ximinez]

account_offers calls the same parsing functions as account_info, etc., and so can take the strict parameter. Could you update the usage to indicate that? (owner_info also can take strict, but it's not deprecated, so don't add it.)

[movitto]

@ximinez updated the account_offers usage output to reflect support for the 'strict' param & updated parseAccountItems comments pertaining to other relavant commands. Let me know if there is anything else!

[scottschurr]

It would be good to update RPCCall_test.cpp.

For example with this pull request the comment here needs to be updated: https://github.com/ripple/rippled/blob/develop/src/test/rpc/RPCCall_test.cpp#L472

And here's a test case that maybe should work but doesn't: https://github.com/ripple/rippled/blob/develop/src/test/rpc/RPCCall_test.cpp#L490

[movitto]

@scottschurr Fixed the broken tests and updated the outdated comments

[ximinez]

Looks good and updated tests appear to be passing.

[scottschurr]

The changes look pretty good to me. I'd like to have @mDuo13 weigh in also since this change affects his documentation.

I do have a couple of minors nit that you could not possibly have known about:

1. You updated the "account_currencies: too many arguments" test to be "current ledger". In the process we lost the "too many arguments" coverage for account_currencies. It would be good to add the "too many arguments" test case back in.

2. The tests are structured, within each command, so the good parses go first followed by bad parses. By leaving what used to be "too many arguments" in place, there's a good parse test in the middle of the bad parse tests. I'd like to move the newly good parse case up with the other good parse cases.

You can see an example the changes I'm proposing here: scottschurr@56c8c79. Feel free to cherry-pick that commit if you wish. Let me know if you have any questions. Thanks.

[movitto]

@scottschurr cherry picked and tried it out, LGTM

[scottschurr]

👍 LGTM. I would like to see @mDuo13 weigh in, however. Thanks.

[mDuo13]

For the strict parameter, what do you type on the commandline to set it to true? Is it 1, true or the word strict?

(I'm just curious, as this has been a point of confusion on past commandline syntax and I don't have a clear answer on which one you should use.)

[scottschurr]

@mDuo13, for the command line you type the word "strict" I believe. @movitto, please correct me if I'm wrong.

[movitto]

@scottschurr yes on the command line "strict" (by itself) should be specified

[scottschurr]

@mDuo13, I haven't seen any objections from you yet. So you're good with this pull request? If so I think it can be marked PASSED

[mDuo13]

👍

[scottschurr]

Sorry, my mistake. All of the CI build targets that run the regular unit tests are showing unit test failures. Here's the text of the failures as I'm seeing them for both clang and gcc:

#14 failed: account_currencies: current ledger.: RPCCall_test.cpp(320)
#25 failed: account_info: with ledger index and strict.: RPCCall_test.cpp(529)
#252 failed: owner_info: with ledger index and strict.: RPCCall_test.cpp(5032)
#257 failed: owner_info: invalid ledger selection and strict.: RPCCall_test.cpp(5131)

We need to understand the reason for the unit test failure first. Assuming the test failures are caused by something benign, once the unit tests are patched up I think this pull request will be good to go. Thanks for being patient.

[movitto]

@scottschurr it's hard to determine from the CI engine why exactly those tests are failing, would it be possible to enable more verbose output?

[mellery451]

you might want to try running the unit tests locally (rippled --unittest does the trick), but FWIW the location of the failures is indicated by SOURCE(line_number) here: https://travis-ci.com/ripple/rippled/jobs/275448524#L914-L917

[movitto]

@mellery451 ok thanks, I was able to reproduce the error locally. @scottschurr pushed a followup patch fixing the issue

[codecov-io]

Codecov Report

Merging #3196 into develop will increase coverage by <.01%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3196      +/-   ##
===========================================
+ Coverage    70.33%   70.33%   +<.01%     
===========================================
  Files          675      675              
  Lines        53178    53177       -1     
===========================================
  Hits         37402    37402              
+ Misses       15776    15775       -1

Impacted Files	Coverage Δ
src/ripple/app/main/Main.cpp	40.13% <ø> (ø)	⬆️
src/ripple/net/impl/RPCCall.cpp	94.86% <0%> (+0.13%)	⬆️

---

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 cc4cefa...0bb0527. Read the comment docs.

[scottschurr]

🎉 Thanks for the persistence @movitto. Looks like we're ready for all of this to be squashed.