Partial refactor of code base #170

MrMeemus · 2018-02-23T01:54:20Z

Changes made:

Argument handling

Moved from click to argparse. Still retaining click for it's echo via pager API.
All objects are accessed via the namespace object generated by argparser.
Arg parse can handle defaults better, no longer requiring explicit checks to set another option.
Example: If no server is passed in, we automatically target master. This reduced checks and centralizes all options into a single file.

Tests

Tests have been updated to handle options via a namespace object. This allows each test method to express what options to toggle along with retaining default values.
All mssql client and option creations are centralized and no longer need explicitly set values on all calls unless a non default option is necessary. This is evident in test_mssqlcliclient.py and test_rowlimit.py

Miscellaneous

Methods were renamed for readability.
Added additional layers of abstraction for readability.
Messages logged are trimmed since some did not provide additional value.
Updated mssqlcliclient to support a shallow clone method.
Mssqlcliclient has private methods added to abstract away the connection and query execution API.
sqltoolsclient API are expressed in a class variable so as to avoid any hardcoded JSON RPC calls.
lower case all action for build.py for robustness.

pensivebrian · 2018-02-23T18:43:47Z

build.py

-            targets[action]()
+        current_action = action.lower()
+        if current_action in targets:
+            targets[current_action]()


I think we should validate all actions and fail before we execute any action. #Closed

pensivebrian · 2018-02-23T18:57:48Z

mssqlcli/mssqlclioptionsparser.py

+MSSQL_CLI_RC = u'MSSQL_CLI_RC'
+
+
+def initialize():


We have two public methods that do the same thing: initialize and get_parser. Can we have a single method: create_parser? #Closed

pensivebrian · 2018-02-23T18:59:05Z

mssqlcli/mssqlclioptionsparser.py

+    return args_parser
+
+
+def get_parser():


get_parser implies we are returning an existing instance. Since were creating a new one each call, I'd prefer we name this create_parser or build_parser. #Closed

pensivebrian · 2018-02-23T19:01:04Z

mssqlcli/mssqlcliclient.py

+                            err=True, fg='yellow')
+                logger.info(u'Unable to reset connection to server {0}; database {1}'.format(
+                    mssqlcli_client.server_name,
+                    mssqlcli_client.database))
                exit(1)


I think this logic can be moved into a reset instance method. #Closed

pensivebrian · 2018-02-23T19:02:09Z

mssqlcli/mssqlcliclient.py

@@ -335,19 +342,19 @@ def reset_connection_and_clients(sql_tools_client, *mssqlcliclients):
    try:
        sql_tools_client.shutdown()
        new_tools_client = SqlToolsClient()


A client should not reinitialize the sqltoolsservice process. Can we move this a reset method in the sqltoolsclient client? #Closed

pensivebrian · 2018-02-23T19:11:36Z

mssqlcli/mssqlcliclient.py

+        return owner_uri, error_messages
+
+    def execute_multi_statement(self, query):
+        # Try to run first as special command


From and API standpoint, it would be nice if we had a single public method to execute on or more statements: execute_query. So execute_single_statement would become private. Any downside to this? #Closed

Agreed, no downsides other then background queries will run through additional code path but it wouldn't be harmful.

In reply to: 170340117 [](ancestors = 170340117)

pensivebrian · 2018-02-23T19:18:15Z

class MssqlCli(object):

What do you think about moving MssqlCli into it's own file? #WontFix

Refers to: mssqlcli/main.py:102 in 4eb4555. [](commit_id = 4eb4555, deletion_comment = False)

pensivebrian · 2018-02-23T19:22:10Z

"""

I think this operation should be moved to the MssqlCli object. #Closed

Refers to: mssqlcli/mssqlcliclient.py:338 in 4eb4555. [](commit_id = 4eb4555, deletion_comment = False)

MrMeemus · 2018-02-23T19:28:26Z

@pensivebrian , YES I WANT TO MOVE MSSQLCLI INTO ITS OWN FILE :) , I can do this on the side in a separate PR as I'm prioritizing the packaging work now.

pensivebrian

🕐

Also, what are the code coverage numbers with this change?

MrMeemus · 2018-02-23T23:20:54Z

67% code coverage

pensivebrian · 2018-02-23T23:24:43Z

mssqlcli/main.py


                click.secho('Reconnected!\nTry the command again.', fg='green')
            except Exception as e:
                click.secho(str(e), err=True, fg='red')

+    def reset_connection(self):
+        """


Maybe just call this reset? #Closed

If you change this, please update the method name in the logging below.

In reply to: 170392798 [](ancestors = 170392798)

It sounds more intuitive to have reset_connection because that is explicitly resetting, what do you think?

Having just reset can imply a lot of options to be reset for MssqlCli.

In reply to: 170392798 [](ancestors = 170392798)

pensivebrian · 2018-02-23T23:35:44Z

mssqlcli/main.py

+            self.mssqlcliclient_main.sql_tools_client = self.sqltoolsclient
+            self.mssqlcliclient_main.is_connected = False
+            self.mssqlcliclient_main.owner_uri = generate_owner_uri()
+


I feel like we should clone the existing mssqlclient such that we know we're in a clean state. #Closed

agreed, I like that idea. Will add it next.

In reply to: 170394199 [](ancestors = 170394199)

pensivebrian · 2018-02-23T23:37:25Z

Awesome! I really like how this cleaned things up.

abhisheksinha89 · 2018-02-23T23:41:45Z

Can we file a GitHub issue to completely remove click once we are done with refactoring? #Resolved

MrMeemus · 2018-02-24T00:10:57Z

@abhisheksinha89 #171
#Resolved

abhisheksinha89

* Initial refactor from click to arg parser. * Refactoring mssqlcli client and renaming methods. * Refactoring mssqlcliclient. * Updating tests. * Updating import statement order. * Adding missing param and fixing pgsql ref. * Making build.py argument handling robust. * Addressing CR comments. * Renamed method for resetting connection and updated to use clone method. * Updating log message in reset.

MrMeemus added 7 commits February 20, 2018 10:32

Initial refactor from click to arg parser.

d1a3f78

Refactoring mssqlcli client and renaming methods.

470a91c

Refactoring mssqlcliclient.

2c37f1d

Updating tests.

f78b157

Updating import statement order.

6122a43

Adding missing param and fixing pgsql ref.

5950f60

Making build.py argument handling robust.

4eb4555

MrMeemus requested review from pensivebrian and abhisheksinha89 February 23, 2018 01:54

pensivebrian reviewed Feb 23, 2018

View reviewed changes

pensivebrian requested changes Feb 23, 2018

View reviewed changes

Addressing CR comments.

c4897db

pensivebrian reviewed Feb 23, 2018

View reviewed changes

pensivebrian approved these changes Feb 23, 2018

View reviewed changes

Renamed method for resetting connection and updated to use clone method.

2c45312

Updating log message in reset.

a4dbd39

abhisheksinha89 approved these changes Feb 26, 2018

View reviewed changes

MrMeemus merged commit f060d9b into master Feb 27, 2018

MrMeemus deleted the ron/code_cleanup branch February 27, 2018 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial refactor of code base #170

Partial refactor of code base #170

MrMeemus commented Feb 23, 2018

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

MrMeemus Feb 23, 2018

pensivebrian commented Feb 23, 2018 •

edited by MrMeemus

pensivebrian commented Feb 23, 2018 •

edited

MrMeemus commented Feb 23, 2018 •

edited

pensivebrian left a comment •

edited

MrMeemus commented Feb 23, 2018

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018

MrMeemus Feb 23, 2018

pensivebrian Feb 23, 2018 •

edited

MrMeemus Feb 23, 2018

pensivebrian commented Feb 23, 2018 •

edited

abhisheksinha89 commented Feb 23, 2018 •

edited by MrMeemus

MrMeemus commented Feb 24, 2018 •

edited

abhisheksinha89 left a comment

Partial refactor of code base #170

Partial refactor of code base #170

Conversation

MrMeemus commented Feb 23, 2018

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

MrMeemus Feb 23, 2018

Choose a reason for hiding this comment

pensivebrian commented Feb 23, 2018 • edited by MrMeemus

pensivebrian commented Feb 23, 2018 • edited

MrMeemus commented Feb 23, 2018 • edited

pensivebrian left a comment • edited

Choose a reason for hiding this comment

MrMeemus commented Feb 23, 2018

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018

Choose a reason for hiding this comment

MrMeemus Feb 23, 2018

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018 • edited

Choose a reason for hiding this comment

MrMeemus Feb 23, 2018

Choose a reason for hiding this comment

pensivebrian commented Feb 23, 2018 • edited

abhisheksinha89 commented Feb 23, 2018 • edited by MrMeemus

MrMeemus commented Feb 24, 2018 • edited

abhisheksinha89 left a comment

Choose a reason for hiding this comment

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian commented Feb 23, 2018 •

edited by MrMeemus

pensivebrian commented Feb 23, 2018 •

edited

MrMeemus commented Feb 23, 2018 •

edited

pensivebrian left a comment •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian Feb 23, 2018 •

edited

pensivebrian commented Feb 23, 2018 •

edited

abhisheksinha89 commented Feb 23, 2018 •

edited by MrMeemus

MrMeemus commented Feb 24, 2018 •

edited