Cast row_limit as integer

[ellbosch]

Fixes #444.

The --row-limit arg was breaking mssql-cli because our code attempted to use a number operator on a string. The fix requires casting the parsed option as an integer.

Notes:

• row_limit is validated in the MssqlCli constructor, which uses a new _set_row_limit method to validate a positive integer.
• _set_row_limit calls sys.exit(1) if an invalid value is provided.
• As a consequence to the above, we needed to catch an AttributeException in the __del__ method because shutdown is called on an uninstantiated sqltoolsclient.
• Testing included to hopefully prevent issues like this in the future.

[pensivebrian]

Is this the right layer/place to make the fix? This seems like an option parsing concern and perhaps handled by argparse?

[ellbosch]

So we definitely can add a check in argparse. However, we may want to keep logic elsewhere because we also accept a value from our config if there's no value for the --row-limit arg. Where that somewhere should be is open for debate.

I've considered main.py and the current location in mssqlcli.py. I picked the current location due to easier testability, also it seems like good design to have the constructor handle this logic.

[pensivebrian]

Let's fix this to args parse for now - seems like the right place.

Looks like a there are a number of issues reading values from our config so let's fix those in another PR at a later date.

[ellbosch]

I take back my prior statement—I was able to move logic for checking the config value right in argparse. It seems to work well, once tests pass I'll update the PR.

[pensivebrian]

This makes me nervous. In what situations will we raise an AttributeError?

[ellbosch]

AttributeError is raised f the MssqlCli constructor exits due to invalid parameters. In this case, say we input a series of characters instead of an integer for --row-limit—we exit during class instantiation, and before sqltoolsclient gets instantiated. Therefore the class doesn't possess the sqltoolsclient attribute, thus raising an AttributeError.

[pensivebrian]

Can you add 'row_limit' to the method name? This way, it stands out from being a generic 'check_positive_int' method. Ideally, we have a generic method where check_row_limit calls check_positive_int('row_limit' value), and check positive can be resused.

[ellbosch]

I'm fairly certain recent failures are due to PR validation build pipelines using the master DB, which tests in this branch don't support. Best we wait until #441 is approved.

[pensivebrian]

Can you clarify what you mean by master db? master db should be read only, and tests should use their own database.

[ellbosch]

Can you clarify what you mean by master db? master db should be read only, and tests should use their own database.

Correct—with our stabilize DB PR, tests now generate a test DB if performing operations off of a database. Some tests were still performing commands off of the permanent DB we created, but that will all change once we merge #441.

In anticipation of this change, I set up or PR validation pipelines to no longer use the permanent DB, so we could properly test #441. This impacts other PRs (like this one) so it's best we wait to merge until #441 is complete.