Refactor of the cli #244

bigspider · 2020-10-09T11:16:04Z

Refactored the cli in order to make sure that each command (including its help message) is entirely implemented within a single class.

The base of all the cli commands is the CliCommand class. Each implementation must have a name, and defines the command and parameters of the command by specifying the values passed to getopt, which is the default behavior of the parse_args method; alternatively, the parse_args method could in principle be overridden to define an arbitrary other way of parsing the parameters.

The Cli class is the actual runner of all the commands. It provides a decorator that is applied to each command class to register all the commands. The decorator also does some basic sanity checks on the correctness of the subclass.

For each command (subclass of CliCommand), the docstring is exactly the text that is shown by the help command for it.
Moreover, the show_help function now generates the list of commands by parsing such docstring for each command. For this to work, the line containing "NAME:" in the docstring must be present and properly formatted. If that fails for any command, show_help will raise an error; a test was added to make sure that show_help does not raise.

Closes: #203, #245

sr-gi

First pass, I like the approach even if it's a pretty custom way of building a CLI.

Some general notes:

Given we are making this way more modular, the RPCClient could be moved to it's own file now, alongside to_json and formatted. That would move all the RPC imports and functionality to there. (We may still need to import grpc to handle the grpc.RpcError in Cli, but that can either be left as is, or create a custom exception and wrap the raise around that, so we can import it from the teos_cli.py.
If run is going to be removed, setup.py needs to be updated so the console script calls main instead.
This will need proper testing now that it can be.

teos/cli/teos_cli.py

sr-gi · 2020-10-10T13:42:13Z

teos/cli/teos_cli.py

-        elif command == "get_tower_info":
-            result = rpc_client.get_tower_info()
+    @classmethod
+    def run(rpc_client, opts_args):


If this is a class method, it normally has the cls param first so it can access stuff from the class: run(cls, rpc_client, opt_args).

Indeed, I changed this few times and didn't realize it was wrong in the base class. 26dcfcf
I changed it to @staticmethod to be equal to all the subclasses; not sure I have a good reason to choose between static and class method, so I used the most limiting by default, can be changed later if the need arises.

AFAIK static uses the class in some way, without changing nor depending on any instantiation of it, whereas class is for constructors.

Commented below on static vs class methods; perhaps @classmethod makes slightly more sense here logically, but @staticmethod is more restrictive, so refactoring in that direction is always possible, should we ever have an implementation that wants to access cls.

sr-gi · 2020-10-10T14:09:28Z

teos/cli/teos_cli.py

+        if command_name not in self.COMMANDS:
+            sys.exit("Unknown command. Use help to check the list of available commands")
+
+        cmd = self.COMMANDS[command_name]


May be worth calling it command? cmd reminds me to the command line interface (may apply to other places).

Done in 26dcfcf.

The are still some matches for cmd*

Renamed all occurrences to either command or command_name in aad0a93.

teos/cli/teos_cli.py

sr-gi · 2020-10-10T14:15:55Z

teos/cli/teos_cli.py

-    try:
-        if command == "get_all_appointments":
-            result = rpc_client.get_all_appointments()
+        return getopt(args, cls.shortopts, cls.longopts)


Am I missing something here, or you're not using the opts in any way?

Yes, this class is basically meant to be like an abstract class (initially I tried with the abc module, but then removed it as all the decorators were just adding tons of additional boilerplate, with little apparent benefit) .
CLICommand calls the parse_args method and passes the result as an argument to the corresponding run method (of the subclass).

This can be a normal method then, or abstract if you don't want instantiation. classmethod is normally meant for factories / constructors.

All the methods of CLICommand and subclasses must be either static or class method, as no instances are ever created, so there's no value bound to self.
@abstractmethod would require to use the abc package (especially for the CLICommand's run method), which I had initially tried, but didn't see to bring any advantage at the end; which is perhaps not surprising as there are no instances aver created, so not much point in calling things "abstract".

I can see why @classmethod is typically used for factory methods, as it would give access to the constructors without breaking in case of inheritance (which is impossible if you use a @staticmethod); but accessing the class variables from subclasses (shortopts and longopts here) seems a valid use case as well.

sr-gi · 2020-10-10T14:16:28Z

teos/cli/teos_cli.py


-    teos_rpc_host = config.get("RPC_BIND")
-    teos_rpc_port = config.get("RPC_PORT")
+    name = None


I think this variables could be part of __init__, and parse_args won't be a class method.

Uhm, I think that would change the design. My idea here is that the subclasses of CLICommand are never instantiated, so there are only static or class methods. parse_args in CLICommand gives a reasonable default behavior (parsing with getopt, but subclasses might in principle override that with a completely different parse_args, which should nevertheless be stateless. This is also necessary if we want to register the commands with a decorator.

Then you can at least remove shortopts and longopts, since they are never used. The latter is also setup to default for getopt, so doing:

return getopt(args, "")

Should do.

As discussed in chat, that's there to provide the default behavior for subclasses in case they want to still use getopt but change just the shortopts or longopts.

bigspider · 2020-10-12T10:57:03Z

Should have addressed most of the requested changes.
Renamed main to run so the entrypoint is consistent with the teos and the client, rather than fixing setup.py.

I added some tests for the CLI class; basically they are just mocking the rpc_client and checking that it is correctly proxying requests there. If the behavior of the CLI becomes more complex, it might make sense to remove all the sys.exit() calls from the CLI, and instead use return values (or errors) to detect various conditions; for now, it didn't seem worth it.

There are no tests for the code inside run() yet; it could make sense to also refactor the argument parsing inside a method in order to make it testable. Let me know if you think it should be done before merging this one, or I'll open a new ticket.

sr-gi

Looks better now. Some additional comments:

There are still places using cmd or cmd_*.
I think more testing is required:
- The RPCClient is not tested for example, nor are some of the negative or command specific cases (e.g. test get_user with an invalid userid).
- CLI in general is almost non-tested for invalid return types.
- ...
  Covering the run method and not exiting straightaway can be covered on a followup I think.

teos/cli/rpc_client.py

teos/cli/teos_cli.py

sr-gi · 2020-10-13T11:41:14Z

teos/cli/teos_cli.py

-    try:
-        if command == "get_all_appointments":
-            result = rpc_client.get_all_appointments()
+        return getopt(args, cls.shortopts, cls.longopts)


This can be a normal method then, or abstract if you don't want instantiation. classmethod is normally meant for factories / constructors.

sr-gi · 2020-10-13T12:11:56Z

teos/cli/teos_cli.py


-    teos_rpc_host = config.get("RPC_BIND")
-    teos_rpc_port = config.get("RPC_PORT")
+    name = None


Then you can at least remove shortopts and longopts, since they are never used. The latter is also setup to default for getopt, so doing:

return getopt(args, "")

Should do.

test/teos/unit/cli/test_teos_cli.py

bigspider · 2020-10-13T15:53:56Z

Opened a separate issue #245 for the RPCClient. Not super urgent imho, as most of the commands just proxy to the actual rpc, so not a lot to test there. Might be more interesting to have fully end-to-end tests where some commands are done via the cli.

I suppose some more testing could be done for CLI.run (especially failure cases), but all the tests for commands are actually calling CLI.run that dispatches to the command; therefore the positive case is tested. To test the failure cases, I suppose we should refactor it so that the sys.exit is not called from within CLI.

sr-gi · 2020-10-15T12:37:14Z

I think #245 could be tackled here. May even be worth also adding the sys.exit bit so we can add proper testing to the whole thing.

This would need squashing btw. I would suggest current code can go in one, given it is pretty self contained, and test in another one. The sys.exit bit could go in a separate one since it builds on top of the code improvements.

sr-gi

Some git housekeeping, small tweaks and some nits.

Should be mergeable after this.

There are several files here being change for rearranging comments / docs. This should go to a separate PR so we don't mess with history in unrelated commits. Feel free to open an issue to keep track of what we see that is not properly formatted and we can go and do it all in bulk. For this specific PR, I've marked the ones for this PR so they can be rebased out.
Modify lines 61 and 114 for teos_cli.py to remove the references to logging, given there is no logging in the cli anymore. Add them to 855ad21.
Regarding tests:
- The last two commits (without taking the linting into account) are a bit intertwined. I think they can either be squased or split so one covers unit and the other covers e2e. If they are separated,the one covering unit may be squashable with e3a3250.
- In cli_e2e, you could have a global variable keeping track of what has been sent to the tower, so you could later check that the data queried to the tower is correct (in test_get_tower_info for example). Since this is being kept track by global variables, it should work no matter if the tests are run independently or as a suite.
- Also in cli_e2e, since you are registering users to check they are in the tower response once queried, why not check they are not there before the registration (for completeness basically).

sr-gi · 2020-11-06T14:04:58Z

teos/appointments_dbm.py

@@ -29,7 +29,7 @@ class AppointmentsDBM(DBManager):
    Args:
        db_path (:obj:`str`): the path (relative or absolute) to the system folder containing the database. A fresh
            database will be created if the specified path does not contain one.
-            
+


Add to a separate formatting issue/PR

Opened an issue to keep track of these: #255

sr-gi · 2020-11-06T14:05:02Z

teos/chain_monitor.py

-        Adds a new block hash to the internal queue of the  :obj:`ChainMonitor` and the internal state. The state contains
-        the list of ``last_tips`` to prevent notifying about old blocks. ``last_tips`` is bounded to
+        Adds a new block hash to the internal queue of the  :obj:`ChainMonitor` and the internal state. The state
+        contains the list of ``last_tips`` to prevent notifying about old blocks. ``last_tips`` is bounded to


Add to a separate formatting issue/PR

Opened an issue to keep track of these: #255

teos/gatekeeper.py

sr-gi · 2020-11-06T14:07:04Z

teos/watcher.py

@@ -673,8 +673,8 @@ def get_user_info(self, user_id):
            user_id (:obj:`str`): the id of the requested user.

        Returns:
-            :obj:`UserInfo <teos.gatekeeper.UserInfo> or :obj:`None`: The user data if found. :obj:`None` if not found, or
-            the ``user_id`` is invalid.
+            :obj:`UserInfo <teos.gatekeeper.UserInfo> or :obj:`None`: The user data if found. :obj:`None` if not found,


Add to a separate formatting issue/PR

Opened an issue to keep track of these: #255

teos/cli/rpc_client.py

test/teos/e2e/test_cli_e2e.py

test/teos/unit/cli/test_rpc_client.py

test/teos/e2e/conftest.py

bigspider · 2020-11-12T05:43:11Z

I was getting lost with trying to get a meaningful history, so I instead squashed all the work on tests into 6f03da7, including the previous changes from PR review.

About the comments in the review:

In cli_e2e, you could have a global variable keeping track of what has been sent to the tower, so you could later check that the data queried to the tower is correct (in test_get_tower_info for example). Since this is being kept track by global variables, it should work no matter if the tests are run independently or as a suite.

The numbers for n_watcher_appointments and n_responder_trackers can easily be tracked only if at the end of each test you make sure that all the appointments go back to zero, as they change when appointments are triggered or expire; hard to keep track reliably imho and it makes the code of tests hard to track. We could track n_registered_users, but currently I'm just registering one user anyway (and to make sure the correct answer is "1", we would have to register the user within test_get_tower_info as well (or rely on other tests calling it before, while now each test in test_cli_e2e can be successfully ran individually.
Tbh, I'm also generally uncomfortable with using global variables in tests, unless they are basically constants for the whole module.
If there is any changing state, it should be encapsulated somewhere, and not managed from within tests, as that makes tests very hard to follow.

Also in cli_e2e, since you are registering users to check they are in the tower response once queried, why not check they are not there before the registration (for completeness basically).

The registration is done with teos client, not with the cli; so I think this test would be out of place in cli_e2e.

sr-gi

LGTM

… sys.exit

…tions in CLI.run

- Added a basic test suite for RPC_client. - Renamed test_basic_e2e to test_client_e2e. - Restructured e2e tests to support different e2e test modules. - Added e2e tests using teos_cli commands.

bigspider force-pushed the 203-cli-refactor branch 2 times, most recently from fbe7209 to 3aa8318 Compare October 9, 2020 12:56

bigspider marked this pull request as ready for review October 9, 2020 22:23

bigspider force-pushed the 203-cli-refactor branch from 9adca26 to 6dde7ab Compare October 9, 2020 22:27

bigspider requested a review from sr-gi October 9, 2020 22:28

sr-gi suggested changes Oct 10, 2020

View reviewed changes

bigspider force-pushed the 203-cli-refactor branch 2 times, most recently from d860b22 to a28c752 Compare October 12, 2020 10:50

bigspider requested a review from sr-gi October 12, 2020 22:32

sr-gi suggested changes Oct 13, 2020

View reviewed changes

bigspider force-pushed the 203-cli-refactor branch from 5267444 to aad0a93 Compare October 13, 2020 13:56

bigspider mentioned this pull request Oct 13, 2020

Add tests for RPCClient #245

Open

bigspider requested a review from sr-gi October 13, 2020 15:54

bigspider force-pushed the 203-cli-refactor branch 6 times, most recently from 793ea92 to e366ea4 Compare October 26, 2020 05:21

bigspider force-pushed the 203-cli-refactor branch 5 times, most recently from 708ff4b to 7c0e939 Compare November 5, 2020 03:48

sr-gi suggested changes Nov 6, 2020

View reviewed changes

bigspider force-pushed the 203-cli-refactor branch 2 times, most recently from 9e4b087 to 6b8b597 Compare November 12, 2020 05:10

bigspider force-pushed the 203-cli-refactor branch from 6b8b597 to 0fd765a Compare November 12, 2020 05:30

bigspider force-pushed the 203-cli-refactor branch from 0fd765a to 1e8b12b Compare November 12, 2020 14:10

sr-gi approved these changes Nov 12, 2020

View reviewed changes

bigspider added 4 commits November 13, 2020 12:33

Refactoring the Cli to have structured command code.

9d39204

Changed CLI and commands to always return a string instead of calling…

f19169a

… sys.exit

Adapted tests to changes in return value; added tests for error condi…

ccca45b

…tions in CLI.run

Major work on cli and RPCClient tests

c0f1e68

- Added a basic test suite for RPC_client. - Renamed test_basic_e2e to test_client_e2e. - Restructured e2e tests to support different e2e test modules. - Added e2e tests using teos_cli commands.

sr-gi force-pushed the 203-cli-refactor branch from 1e8b12b to c0f1e68 Compare November 13, 2020 11:41

sr-gi merged commit 651471e into talaia-labs:master Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of the cli #244

Refactor of the cli #244

bigspider commented Oct 9, 2020 •

edited

Loading

sr-gi left a comment

sr-gi Oct 10, 2020

bigspider Oct 11, 2020

sr-gi Oct 13, 2020

bigspider Oct 13, 2020

sr-gi Oct 10, 2020

bigspider Oct 11, 2020

sr-gi Oct 13, 2020

bigspider Oct 13, 2020

sr-gi Oct 10, 2020

bigspider Oct 11, 2020

sr-gi Oct 13, 2020

bigspider Oct 13, 2020

sr-gi Oct 10, 2020

bigspider Oct 11, 2020

sr-gi Oct 13, 2020

bigspider Oct 13, 2020

bigspider commented Oct 12, 2020

sr-gi left a comment

sr-gi Oct 13, 2020

sr-gi Oct 13, 2020

bigspider commented Oct 13, 2020

sr-gi commented Oct 15, 2020

sr-gi left a comment

sr-gi Nov 6, 2020

sr-gi Nov 12, 2020

sr-gi Nov 6, 2020

sr-gi Nov 12, 2020

sr-gi Nov 6, 2020

sr-gi Nov 12, 2020

bigspider commented Nov 12, 2020

sr-gi left a comment

Refactor of the cli #244

Refactor of the cli #244

Conversation

bigspider commented Oct 9, 2020 • edited Loading

sr-gi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigspider commented Oct 12, 2020

sr-gi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigspider commented Oct 13, 2020

sr-gi commented Oct 15, 2020

sr-gi 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

bigspider commented Nov 12, 2020

sr-gi left a comment

Choose a reason for hiding this comment

bigspider commented Oct 9, 2020 •

edited

Loading