Configure placement of show_query in non-stream output

[Coteh]

First of all, just want to preface by saying that SQLTools is an amazing tool! I use it every day at work and it makes executing query snippets much easier and speeds up my workflow tremendously. I have some ideas that would improve my specific workflow that I believe would also improve the project as a whole. However, I want to spend some time getting acquainted with the codebase before I go forth with suggesting any of these ideas.

In the meantime, I am just getting my feet wet with the codebase and I figured I go ahead and work on this small but significant change to the show_query setting of the plugin. In the current version, when enabling show_query, it will place the query information on top of the results if in regular output mode, and if in stream output mode, places it on the bottom of the results after the stream finishes. However, I wanted to have the option of specifying where this query information gets displayed when in non-stream output mode. Hence, I have changed the show_query setting to now be comprised of two sub-settings: enabled and placement.

• enabled can either be true or false, much like the original show_query boolean setting was.
• placement can either be "top" or "bottom". If this setting is omitted, then it will default to "top". If it's set to a string that is not "top" or "bottom" the string will be formatted but not printed. I wasn't sure what the best option would be in this particular case (I originally thought of defaulting it to "top"), so I want to know what you guys think would be best in this case. Also, this setting is ignored when in stream output mode: it will always output to the bottom of the stream when in this mode.

So overall, what do you guys think of this change? It's a small change, but definitely a critical change, as it will break all settings files until show_query is changed to reflect the new format. Thus, I can understand why it may potentially be an issue. Also, if you guys have a better idea of how to go about implementing the placement setting without breaking existing show_query settings, let me know.

[mtxr]

Great job!!

Just a question, does it still working for other versions? What will happen if user still using True or False?

Also, could you update de docs at gh-pages branch?

[Coteh]

So right now, if show_query is a bool, it will error out whenever a query is executed because it's expecting show_query to be an object and not a bool. As a result, the query results won't display. I think I can add in a check if it's an object or a bool and deal with either case accordingly.

Also, there's another type of error that occurs when system setting's show_query is a bool and the user setting's show_query is an object, and vice versa. The merge method in Utils gets confused when the types are mismatched. Not sure if much can be done about that besides just updating the system's setting and getting the users to update their user setting.

Also, yes I will update the doc! :)

[tkopets]

Sorry guys for this short communication (I don't have much time lately).

Maybe a multi-valued option would work in this case (same as autocompletion setting) - could be be false, 'basic', 'smart' ? (probably you can explore the related code for autocompletion setting to see how it works)

So you can make it like this: "show_query": false or "show_query": "top" or "show_query": "bottom" Also you can add an additional check in plugin code if show_query is not top or bottom to default to previous (as it used to be) placement.

This way existing users will feel no difference either if they had show_query: false or show_query: true.

It would be a very bad user experience if we would ask or expect all users of SQLTools to modify/fix their settings file because of new functionality.

[tkopets]

Also, thank you very much for your contribution and your feedback!

I will have more time later today or tomorrow to take a look into code / test with different scenarios.
One of the things to keep in mind is that process can be killed after thread_timeout seconds so you have to think carefully if your code will be run if that case.
Also, it is a good idea to test your functionality in combination with thread_timeout on Windows and Mac/Linux (as Windows platform has slightly different handling of how the process is killed).

[Coteh]

You're totally right, it is not a good idea to break backwards compatibility so willy-nilly, especially since this is not supposed to be a major functionality change of any kind. I have modified my code to now make it similar to the autocompletion setting.

I have also considered thread_timeout and I don't think my code will be affected by a timeout. However, printing the query details at the bottom will start on the same line as the last line of results before the timeout if a timeout occurred. I tried to find a definitive way of telling if the subprocess was closed due to a timeout, but the best I could do right now is to check if the subprocess was set to None, as that happens if timeout occurred. Once I determine that, I will print a newline if there was output written, otherwise I will leave it. It would be handy if there was some sort of query result information attached to the Command class that I can use to determine if the command finished successfully, if there was an error, or if there was a timeout, but I can't think of a good way to implement this right now and if anything else besides my timeout check would even use it.

My addition appears to be working correctly after a timeout on Windows. I can test a Linux machine tomorrow to make sure things are working correctly on there too.

[tkopets]

@Coteh Thank you for putting thought and effort to make it high quality and the right way!

Sorry, but I didn't had much time to review the code itself, but I will try to do it tomorrow.

[Coteh]

Just tested it out using a Linux virtual machine, doesn't appear to be any problems involving thread_timeout. Everything appears to be running normally.

[tkopets]

@Coteh Finally, I have reviewed and did a quick testing of this new functionality! This pull request is a good quality work and is ready to be merged.

[mtxr]

merging upon @tkopets review and approval