Cache CLI input values #1496

uatuko · 2018-06-22T22:11:05Z

Description of the Change

Cache CLI input values to make it more convenient (#827).

There are two use cases.

Make is possible to use default values provided during creation of a iroha_cli::interactive::ParamsMap - If a default value is provided (e.g. default host/port), that will be used when the user input is empty.
Cache any user inputs - When an input is captured for a parameter, that value is cached and displayed on the input prompt for that parameter. If the user input is empty, cache value will be used.

Benefits

Get rid of annoying repeated host/port entries when attempting to send requests to Iroha (among other inputs) and make the CLI less frustrating to use.

Possible Drawbacks

There's no option to remove a cached value other than replacing it with a new value. However, the CLI doesn't allow empty inputs so this shouldn't be a problem.

Usage Examples or Tests

Scenario 1 - Using default values

There's no user input for Peer port (50051), default is used (prompt displays the default values).

$ ../build/bin/iroha-cli -account_name admin@test
Welcome to Iroha-Cli. 
Choose what to do:
1. New transaction (tx)
2. New query (qry)
3. New transaction status request (st)
> : 2
Choose query: 
1. Get Account Information (get_acc)
2. Get Account's Asset Transactions (get_acc_ast_tx)
3. Get Account's Signatories (get_acc_sign)
4. Get Account's Assets (get_acc_ast)
5. Get all current roles in the system (get_roles)
6. Get information about asset (get_ast_info)
7. Get Account's Transactions (get_acc_tx)
8. Get Transactions by transactions' hashes (get_tx)
9. Get all permissions related to role (get_role_perm)
0. Back (b)
> : 7
Requested account Id: admin@test
Query is formed. Choose what to do:
1. Send to Iroha peer (send)
2. Save as json file (save)
0. Back (b)
> : 1
Peer address (0.0.0.0): localhost
Peer port (50051):
[2018-06-22 22:42:44.265421000][th:5963568][info] QueryResponseHandler [Transaction]
[2018-06-22 22:42:44.265988000][th:5963568][info] QueryResponseHandler -Hash- ed98fb922acad35d9262d56115daba6b092722f60523a9e3e03e642cc2364fd3
[2018-06-22 22:42:44.266060000][th:5963568][info] QueryResponseHandler -Creator Id- admin@test
[2018-06-22 22:42:44.266081000][th:5963568][info] QueryResponseHandler -Created Time- 1529697208667
[2018-06-22 22:42:44.266162000][th:5963568][info] QueryResponseHandler -Commands- 3
[2018-06-22 22:42:44.267579000][th:5963568][info] QueryResponseHandler  CreateAsset: [asset_name=coolcoin, domain_id=test, precision=2, ] 
[2018-06-22 22:42:44.267692000][th:5963568][info] QueryResponseHandler  AddAssetQuantity: [account_id=admin@test, asset_id=coolcoin#test, amount=Amount: [intValue=50050, precision=2, ], ] 
[2018-06-22 22:42:44.267753000][th:5963568][info] QueryResponseHandler  TransferAsset: [src_account_id=admin@test, dest_account_id=test@test, asset_id=coolcoin#test, description=, amount=Amount: [intValue=10050, precision=2, ], ] 
--------------------

Scenario 2 - Using cached values

There aren't any user inputs for Requested account Id (admin@test), Peer address (localhost) and Peer port (50051) , cached valued are used. Note the prompt displays the cached values.

Choose what to do:
1. New transaction (tx)
2. New query (qry)
3. New transaction status request (st)
> : 2
Choose query: 
1. Get Account Information (get_acc)
2. Get Account's Asset Transactions (get_acc_ast_tx)
3. Get Account's Signatories (get_acc_sign)
4. Get Account's Assets (get_acc_ast)
5. Get all current roles in the system (get_roles)
6. Get information about asset (get_ast_info)
7. Get Account's Transactions (get_acc_tx)
8. Get Transactions by transactions' hashes (get_tx)
9. Get all permissions related to role (get_role_perm)
0. Back (b)
> : 7
Requested account Id (admin@test): 
Query is formed. Choose what to do:
1. Send to Iroha peer (send)
2. Save as json file (save)
0. Back (b)
> : 1
Peer address (localhost): 
Peer port (50051): 
[2018-06-22 22:42:51.678041000][th:5963568][info] QueryResponseHandler [Transaction]
[2018-06-22 22:42:51.678202000][th:5963568][info] QueryResponseHandler -Hash- ed98fb922acad35d9262d56115daba6b092722f60523a9e3e03e642cc2364fd3
[2018-06-22 22:42:51.678225000][th:5963568][info] QueryResponseHandler -Creator Id- admin@test
[2018-06-22 22:42:51.678237000][th:5963568][info] QueryResponseHandler -Created Time- 1529697208667
[2018-06-22 22:42:51.678266000][th:5963568][info] QueryResponseHandler -Commands- 3
[2018-06-22 22:42:51.678312000][th:5963568][info] QueryResponseHandler  CreateAsset: [asset_name=coolcoin, domain_id=test, precision=2, ] 
[2018-06-22 22:42:51.678363000][th:5963568][info] QueryResponseHandler  AddAssetQuantity: [account_id=admin@test, asset_id=coolcoin#test, amount=Amount: [intValue=50050, precision=2, ], ] 
[2018-06-22 22:42:51.678408000][th:5963568][info] QueryResponseHandler  TransferAsset: [src_account_id=admin@test, dest_account_id=test@test, asset_id=coolcoin#test, description=, amount=Amount: [intValue=10050, precision=2, ], ] 
--------------------
Choose what to do:
1. New transaction (tx)
2. New query (qry)
3. New transaction status request (st)
> :

Signed-off-by: Uditha Atukorala <[email protected]>

l4l

Use clang-format please

l4l · 2018-06-24T13:31:56Z

iroha-cli/interactive/interactive_common_cli.hpp

@@ -73,6 +77,8 @@ namespace iroha_cli {
    ParamsMap getCommonParamsMap(const std::string &default_ip,
                                 int default_port);



Would be nice to add doc comment

l4l · 2018-06-24T13:36:23Z

iroha-cli/interactive/interactive_common_cli.hpp

    // Description of parameters
-    using ParamsDescription = std::vector<std::string>;
+    using ParamsDescription = std::vector<std::shared_ptr<ParamData>>;


Why shared_ptr?

So I can use and update the cache. I tried passing ParamData by refs instead of a pointer but doesn't seem to work with boost::optional (or I couldn't get it to work). I can give it another go to avoid using a pointer if that's preferred.

I mean why not just std::vector<ParamData>?

Because it gets copied and I lose the reference to the initial objection when trying to update the cache (interactive_common_cli.cpp:148).

Hmm, it looks useless. Parameter in function is auto, not auto &, so modification of param bring nothing

I'll give another go at using std::vector<ParamData>. IIRC I couldn't get it to compile when tried to return a reference in findInHandlerMap.

l4l · 2018-06-24T13:39:03Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

          // commonParamsMap
      };
    }

+    ParamsDescription makeParamsDescription(std::vector<std::string> params) {
+        ParamsDescription data;


Consider using:

return std::accumulate( params.begin(), params.end(), ParamsDescription{}, [](auto &&acc, auto &el) { acc.push_back(std::make_shared<ParamData>(std::make_pair(el, ""))); return std::forward<decltype(acc)>(acc); });

l4l · 2018-06-24T13:39:49Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

@@ -62,11 +72,11 @@ namespace iroha_cli {
    }

    void printCommandParameters(std::string &command,
-                                std::vector<std::string> parameters) {
+                                ParamsDescription parameters) {


should be const ParamsDescription&

l4l · 2018-06-24T13:40:58Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

@@ -87,12 +97,20 @@ namespace iroha_cli {
      return line;
    }

+    boost::optional<std::string> promptString(ParamData param) {
+        std::string message = std::get<0>(param);


I consider adding a new structure, because std::get say nothing about the semantic

l4l · 2018-06-24T13:42:30Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

@@ -87,12 +97,20 @@ namespace iroha_cli {
      return line;
    }

+    boost::optional<std::string> promptString(ParamData param) {
+        std::string message = std::get<0>(param);
+        if (not std::get<1>(param).empty()) {


std::accumulate is also applicable here

Don't think it is appropriate to use std::accumulate here. param is just a std::pair (and changing to a struct).

Oops, indeed. I though there was a loop, nvm then

l4l · 2018-06-24T13:44:24Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

-                        auto val = promptString(param);
-                        if (val and not val.value().empty()) {
-                          params.push_back(val.value());
+                        auto val = promptString(*param);


Prefer using monadic bind operator

promptString(*param) | [&](auto &val) { if (not val.empty()) { ... };

I'm not sure how to use this feature, I get a compile error 😟.

../iroha-cli/interactive/impl/interactive_common_cli.cpp:151:28: error: invalid operands to binary expression ('boost::optional<std::string>' (aka 'optional<basic_string<char, char_traits<char>, allocator<char> > >') and '(lambda at ../iroha-cli/interactive/impl/interactive_common_cli.cpp:151:30)') promptString(*param) | [&](auto &val) { ~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~

Help please?

Do not forget to include "common/types.hpp"

The operator itself is in the namespace, so you may use using namespace iroha; inside the function

Was missing using namespace iroha;. Thanks.

l4l · 2018-06-24T13:46:54Z

iroha-cli/interactive/impl/interactive_query_cli.cpp

+          {GET_ROLES, makeParamsDescription({})},
+          {GET_AST_INFO, makeParamsDescription({ast_id})},
+          {GET_ROLE_PERM, makeParamsDescription({role_id})}
+          // query_params_map_


i think it can be removed

Could you please clarify, not sure what you meant here.

I mean the line with a comment. But you don't really have to remove it since the PR is about different things

No, this comment is needed for better formatting

l4l · 2018-06-24T13:48:39Z

iroha-cli/interactive/interactive_common_cli.hpp

@@ -46,8 +47,11 @@ namespace iroha_cli {
      RESULT
    };

+
+    // Parameter prompt and default/cache value pair
+    using ParamData = std::pair<std::string, std::string>;


consider declaring struct ParamData instead

l4l · 2018-06-24T13:49:09Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

          // commonParamsMap
      };
    }

+    ParamsDescription makeParamsDescription(std::vector<std::string> params) {


const std::vector<std::string> &

uatuko · 2018-06-24T16:09:48Z

@l4l, if I use clang-format (e.g. clang-format -i -style=Google interactive_common_cli.hpp) the entire file seem to get reformatted. Should I do it anyway or am I using clan-format incorrectly?

l4l · 2018-06-24T16:19:16Z

@uditha-atukorala we have minor differences with google code style (in terms of clang-format) actually. They are listed in config, so prefer using -style=file (or smth like that)

Signed-off-by: Uditha Atukorala <[email protected]>

l4l · 2018-06-25T06:22:19Z

iroha-cli/interactive/interactive_common_cli.hpp

    // Description of parameters
-    using ParamsDescription = std::vector<std::string>;
+    using ParamsDescription = std::vector<std::shared_ptr<ParamData>>;


As a continuation of the previous discussion
boost::optional can handle lvalue refs. Try this diff out: curl http://termbin.com/kzb6 | git apply

Signed-off-by: Uditha Atukorala <[email protected]>

l4l

lgtm

l4l · 2018-06-25T14:54:02Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

+                             [](auto &&acc, auto &el) {
+                               acc.push_back(ParamData({el, {}}));
+                               return std::forward<decltype(acc)>(acc);
+                               ;


Remove that ; plz

l4l · 2018-06-25T14:54:10Z

iroha-cli/interactive/interactive_common_cli.hpp

-    // Parameter prompt and default/cache
+    /**
+     * Data structure for parameter data
+     *


redundant line

Signed-off-by: Uditha Atukorala <[email protected]>

sorabot · 2018-06-25T16:50:50Z

SonarQube analysis reported 2 issues

interactive_common_cli.cpp#L43: Return value of function to_string() is not used.
interactive_transaction_cli.hpp#L145: Unused private function: 'InteractiveTransactionCli::parseRevokePermission'

grimadas

Great job, thanks for your contribution

grimadas · 2018-06-26T01:47:47Z

iroha-cli/interactive/impl/interactive_common_cli.cpp

-        std::string line, std::string command_name, ParamsMap params_map) {
-      auto params_description =
-          findInHandlerMap(command_name, std::move(params_map));
+        std::string line, std::string command_name, ParamsMap &params_map) {


I guess it should be const ParamsMap &

Actually not, it is intentional it to be not const ParamsMap & since it gets updated with user input values.

Thanks. Missed that

grimadas · 2018-06-26T01:48:38Z

iroha-cli/interactive/interactive_common_cli.hpp

@@ -136,7 +170,7 @@ namespace iroha_cli {
     * @return vector with needed parameters
     */
    boost::optional<std::vector<std::string>> parseParams(
-        std::string line, std::string command_name, ParamsMap params_map);
+        std::string line, std::string command_name, ParamsMap &params_map);


Also const ParamsMap &

As above, params_map gets updated.

grimadas · 2018-06-26T01:48:52Z

iroha-cli/interactive/interactive_common_cli.hpp

-    boost::optional<V> findInHandlerMap(K command_name,
-                                         std::unordered_map<K, V> params_map) {
+    boost::optional<V &> findInHandlerMap(
+        K command_name, std::unordered_map<K, V> &params_map) {


As above, params_map gets updated.

grimadas · 2018-06-26T01:49:20Z

iroha-cli/interactive/interactive_common_cli.hpp

@@ -211,7 +245,7 @@ namespace iroha_cli {
        C class_pointer,
        std::string &line,
        std::unordered_map<std::string, V> &parsers_map,
-        ParamsMap params_map) {
+        ParamsMap &params_map) {


const ParamsMap &

As above, params_map gets updated.

Signed-off-by: Uditha Atukorala <[email protected]>

uatuko added 4 commits June 22, 2018 15:45

(iroha-cli) cache input params

8f2e121

Signed-off-by: Uditha Atukorala <[email protected]>

(iroha-cli) fix refs for input params

897cfe9

Signed-off-by: Uditha Atukorala <[email protected]>

(iroha-cli) cleanup

0c2eaf3

Signed-off-by: Uditha Atukorala <[email protected]>

(iroha-cli) add missing headers

af57b48

Signed-off-by: Uditha Atukorala <[email protected]>

l4l added needs-review pr awaits review from maintainers cli command line interface new-feature labels Jun 24, 2018

l4l requested review from l4l and grimadas June 24, 2018 13:16

l4l suggested changes Jun 24, 2018

View reviewed changes

uatuko added 2 commits June 24, 2018 21:09

(iroha-cli) PR review changes

bd6c989

Signed-off-by: Uditha Atukorala <[email protected]>

clang-format

6b28bbd

Signed-off-by: Uditha Atukorala <[email protected]>

l4l reviewed Jun 25, 2018

View reviewed changes

uatuko added 3 commits June 25, 2018 14:31

(iroha-cli) drop shared_ptr usage

634fa81

Signed-off-by: Uditha Atukorala <[email protected]>

(iroha-cli) PR review changes (monadic bind operator)

2856e9c

Signed-off-by: Uditha Atukorala <[email protected]>

(iroha-cli) doc comments

df53fcd

Signed-off-by: Uditha Atukorala <[email protected]>

l4l approved these changes Jun 25, 2018

View reviewed changes

(iroha-cli) cleanup

047f633

Signed-off-by: Uditha Atukorala <[email protected]>

grimadas suggested changes Jun 26, 2018

View reviewed changes

grimadas approved these changes Jun 26, 2018

View reviewed changes

l4l removed the needs-review pr awaits review from maintainers label Jun 26, 2018

l4l merged commit 04c3bea into hyperledger-iroha:develop Jun 26, 2018

uatuko deleted the feature/cache-cli-input-params branch June 26, 2018 10:10

neewy mentioned this pull request Jul 25, 2018

Hyperledger Iroha v1.0 beta-4 version #1597

Merged

l4l pushed a commit that referenced this pull request Jul 25, 2018

Cache CLI input values (#1496)

000301f

Signed-off-by: Uditha Atukorala <[email protected]>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Cache CLI input values (#1496)

49397a4

Signed-off-by: Uditha Atukorala <[email protected]>

		@@ -73,6 +77,8 @@ namespace iroha_cli {
		ParamsMap getCommonParamsMap(const std::string &default_ip,
		int default_port);

Cache CLI input values #1496

Cache CLI input values #1496

Conversation

uatuko commented Jun 22, 2018

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests

Scenario 1 - Using default values

Scenario 2 - Using cached values

l4l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l4l Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uatuko commented Jun 24, 2018

l4l commented Jun 24, 2018

Choose a reason for hiding this comment

l4l left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorabot commented Jun 25, 2018

grimadas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l4l Jun 24, 2018 •

edited

Loading