Fixing startupcommands and successful perpetually set as True

[bjornasm]

Description

Startup commands

As seen in #56 there is a wish for having the option to define a set of commands that are to be executed on startup of litecli. For my case this would for instance be .tables - as I always seem to forget their names.

Startupcommands are set in liteclirc, I chose this rather than a new rc file to keep things simple.

# Startup commands
# litecli commands or sqlite commands to be executed on startup.
# some of them will require you to have a database attached. 
# they will be executed in the same order as they appear in the list.
[startup_commands]
#commands = ".tables", "pragma foreign_keys = ON;"

As I wanted to keep in line with the rest of the codebase the commands are executed using sqlexecute.run(command) in the startup_commands() function that loops through all the startup commands at startup. To facilitate for helpful error messaging I have also added check_if_sqlitedotcommand(command) that is invoked inside sqlexecute.run() to check if the command the user is trying to execute is indeed a valid dotcommand, however it is not implemented yet. I though this would be nice so the users doesn't question whether they had a spelling error in their command in those cases.

With this implementation there may be a wish to be able to utilize more of the dot commands or other special commands from SQLite - should we implement more of these into LiteCLI? Or is it to whish to not expand the range of special commands offered?

Successfull perpetually set as True

Queries are amongst others logged into self.query_history which can also be accessed through self.get_last_query. Here they are stored as "Query", ["query", "successful", "mutating"].

successful is set in main.py , successful = False. This was undoubtedly with the intention to set successful = True at successful execution of queries. However I have found that failing queries are set as successful simply because the program will continue to run after the intial res = sqlexecute.run(text) and thus set successful = True

Testcase, hardcoded, from this line:

            successful = False
            start = time()
            text = "this-is-not-a-query;"
            res = sqlexecute.run(text)
            self.formatter.query = text
            successful = True

My fix is to only set successful = True in the subsequent execution logic, often as an else: in the try/except: statements.

If the intention was that the definition of successful = True is that a query is handled in any way this pull request can be disregarded, however please consider this interpretation of the logic as that makes it easier to access failed queries.

Checklist

• I've added this contribution to the CHANGELOG.md file.

Note: One test is failing, this is unrelated to these changes as it fails on the code from the main branch as well, see issue 153

[amjith]

I like the feature. Thanks for sending the PR.

Can you add the startup section in the liteclirc that's in the tests folder?

It'll be good to exercise the code path with a test if possible.

[bjornasm]

@amjith Will do! This pull was just intended to be isolated to the successfull = True part, but I see that the startupcommands also was added when I commited that to branch after the pull request, apologies. Let me remove that from this pull request so you could consider that individually, and make a separate pull request for the startupcommands after the fact. Then I can also do a thorough explanation in its own pull request, and also add the correct part to the liteclirc (and possibly add a test for it?).

[amjith]

No need to break up the prs. Feel free to add it to this one.

…

On Fri, May 5, 2023 at 11:12 PM Bjørnar Brende Smestad < ***@***.***> wrote: @amjith <https://github.com/amjith> Will do! This pull was just intended to be isolated to the successfull = True part, but I see that the startupcommands also was added when I commited that to branch after the pull request, apologies. Do you want me to remove that from this pull request so you could consider that individually, or should I add an explanation of the startup commands part of the pull request to this original post as well as add the code to the test liteclirc? — Reply to this email directly, view it on GitHub <#160 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMA3FDID5ZKTZIL4CEU6DXEXTV3ANCNFSM6AAAAAAXU5Y4RE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bjornasm]

Thank you! I updated my initial post with an introduction to the startup commands. I tried adding a test but failed short on invoking run_cli() without actually invoking the cli. Using the CliRunner() it seems like it does not run the different functions in run_cli(). I am happy to add the test for this, but I think I need some help along the way.

`def test_startup_commands(executor):
m = LiteCli(liteclirc=default_config_file)
assert m.startup_commands['commands'] == ['create table startupcommands(a text)', "insert into startupcommands values('abc')"]

m.connect('_test_db')
m.run_cli()

sql = "select * from startupcommands;"
runner = CliRunner()
result = runner.invoke(cli, args=CLI_ARGS + ["-e", sql])

assert result.exit_code == 0
assert "abc" in result.output`

[amjith]

Doesn't this need a [startup_commands] section?

[bjornasm]

Oh yes, sorry. Fixed that and added a test to check for the startup commands. Now only miss a test for the execution of the startup commands which I am not sure on how I should do - the check itself is fine but not sure how I invoke the cli without the cli taking over.

[amjith]

I thought we had some tests that exercised the run_cli() method but it looks like that's not the case. I'm fine not adding a test for this feature.

Are you able to reproduce the failing test locally? I see that the build is failing in GH actions but I am unable to reproduce it locally.

[bjornasm]

The failing test is mentioned here: #153, this is the test that fails:

def test_auto_escaped_col_names(completer, complete_event):
    text = "SELECT  from `select`"
    position = len("SELECT ")
    result = list(
        completer.get_completions(
            Document(text=text, cursor_position=position), complete_event
        )
    )
    assert (
        result
        == [
            Completion(text="*", start_position=0),
            Completion(text="`ABC`", start_position=0),
            Completion(text="`insert`", start_position=0),
            Completion(text="id", start_position=0),
        ]
        + list(map(Completion, completer.functions))
        + [Completion(text="`select`", start_position=0)]
        + list(map(Completion, sorted(completer.keywords)))
    )

I just made a new clone of the main repo of litecli and ran the tests and it is falling for me (using Python3.11) - so not related to my changes. I was thinking to having a look into why it is failing.

[amjith]

Thanks for contributing the feature.