Cast row_limit as integer

dbcli / mssql-cli

A command-line client for SQL Server with auto-completion and syntax highlighting

BSD 3-Clause "New" or "Revised" License

1.35k stars 192 forks source link

Cast row_limit as integer #448

Closed ellbosch closed 4 years ago

ellbosch commented 4 years ago

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.

ellbosch commented 4 years ago

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 commented 4 years ago

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

ellbosch commented 4 years ago

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.