search

julianhyde / sqlline

Shell for issuing SQL to relational databases via JDBC
BSD 3-Clause "New" or "Revised" License
620 stars 146 forks source link

Sql syntax highlight support #164

Closed snuyanzin closed 6 years ago

snuyanzin commented 6 years ago

Currently there is color property in sqlline but it has nothing in common with syntax highliting... Does it make sense to have a different property to switch on/off sql syntax?

About syntax highlighting it self. Currently have done it in my sandbox in the next way The initial color scheme is defined in Application and could be rewritten the initial color scheme looks like

any suggestions about relating to color scheme improvement are welcome (however it is customizable as other Application stuff) To provide more real example of how it could look like there is a screenshot query_example

snuyanzin commented 6 years ago

Current version from which the screenshot was taken is available at https://github.com/snuyanzin/sqlline/tree/SQLLINE_164 It is already based on #165 and #129

arina-ielchiieva commented 6 years ago

@snuyanzin

  1. what current color property does? Do we need it?
  2. I believe adding ability to override highlight options config in application class is enough, there is no need for specific property.
  3. Chosen color scheme looks good to me.
snuyanzin commented 6 years ago

what current color property does? Do we need it?

As far as I was able to understand in case of color==true the output (but not the input) will be colorized. For instance whatever connectivity errors/absent connection/files/isolation levels will be in red. About green I found that set of properties will be green after !set command, result of sql table (at least in case TableOutputFormat). Thus !set color does nothing with input but could highlight output

julianhyde commented 6 years ago

Very nice. I especially like how you have used the JDBC driver to find keywords.

You have mis-spelled "highlight" as "hightlight" in a few places; please fix.

Let's split the checkstyle upgrade and highlighting into separate commits. (Are they both in #165 right now? I haven't checked whether they are separable in the pull request. If you think they are, I'll just commit checkstyle now.)

Even though checkstyle doesn't enforce it, please change lines like

String[] successfulLinesToCheck = new String[] {

to

String[] successfulLinesToCheck = {

A few ideas how this could be extended:

  1. Highlight based on syntax category. From the JDBC driver, you can find out how identifiers are delimited (double-quote, brackets, back-ticks). The identifier class (which you call the double-quote class) could to however the current SQL dialect represents quoted-identifiers.

  2. Pre-defined and user-defined color schemes. A scheme could be a string of category=color, e.g. keyword=blue,string=green In Application you could define several built-in schemes, e.g. light(keyword=blue,string=green) dark(keyword=red,string=white) Users could over-ride those styles in their sub-class of Application. Or perhaps there could be a colorSchemes variable.

  3. Choose scheme via a variable:

    !set colorScheme dark

Useful if you are running in a terminal that is white text on black background.

  1. Need some more tests for tricky-but-likely scenarios, e.g. a double-quoted identifier with escaped double quotes in it, multi-line comments.

I know these are hard. Which of them, in your opinion, are worth doing?

snuyanzin commented 6 years ago

Thank you for your response.

You have mis-spelled "highlight" as "hightlight" in a few places; please fix.

Yes it was first this ticket related commit and this was fixed in further commits

Let's split the checkstyle upgrade and highlighting into separate commits. (Are they both in #165 right now? I haven't checked whether they are separable in the pull request. If you think they are, I'll just commit checkstyle now.)

No #165 is independent and could be committed separately. By saying based on #165 I mean that I did my local rebase. Just committed to #165 changes related to 

String[] successfulLinesToCheck = new String[] {

Thank you for pointing to this. So if it is possible it would be nice to have checkstyle related PR committed.

About ideas (thank you very much for sharing them):

  1. Yes I thought in a similar way but I guess one of the tricky thing here will be to have it working while having separate connections with different JDBC delimiters. Currently no idea how in this case should be highlighted query mentioned in !all command...
  3. Yes nice ideas for both points. The only issue which probably could be is that each time LineReader should be reinitialized as there is no option in jline to set a new highlighter. Only LineReader reinitialization. However may be it could be done on highlighter level... Need to test it first before saying yes or no.
  4. Agree currently I generated more tests (not everything is still in github) but I have some more ideas about tests, so the amount of syntax highlighting tests will grow.
snuyanzin commented 6 years ago

Some updates from my side

  1. I added support of highlighting based on information we have from JDBC driver (getSQLKeywords, getIdentifierQuoteString) for the current connection. It will requests data once per active connection and also will keep data per connection (except default). So in case of having connections to dbs with different set of keywords the defaultset + additional set for the current active connection will be used. There are test checking it (sqlline.SqlLineHighlighterTest#testH2SqlIdentifierFromDatabase, sqlline.SqlLineHighlighterTest#testH2SqlKeywordsFromDatabase). Also did rename double-quote => sql identifier in the code.
  2. I did some search through internet and was inspired by [1] and [2] color schemes. I added not the same but something more or less similar. So now there are 7 pre-defined color schemes chester/dark/dracula/light/ozbsidian/solarized/vs2010. The default is no schema (as before highlighting). Adding of a new schema now is just adding a new Map.Entry into sqlline.HighlightStyle#NAME2HIGHLIGHT_STYLE.
  3. To switch between schemas the next command could be used !set colorscheme <scheme name>
  4. Added a number of tests for multiline commands, comments, with escaping single quotes, sql identifier quotes (back ticks and double quotes). The tests work for several color schemes (others could be added pretty easy without changing tests).

@julianhyde could you please merge #165 and then I will make a PR for this issue if it is ok

[1] https://github.com/Gillisdc/sqldeveloper-syntax-highlighting [2] https://github.com/ozmoroz/ozbsidian-sqldeveloper

snuyanzin commented 6 years ago

By the way I have a suggestion to use widgets to switch between color schemes. I've just created one, bound it to keyboard combination and it allows me to see the same query with different highlighting without resetting colorscheme property and reentering the query

julianhyde commented 6 years ago

I like the idea of a widget switch between color schemes. Maybe a keystroke could rotate through the available color schemes (there shouldn't be too many). It would be useful if I were giving a presentation.

But there would be a variable for color scheme as well, right? Then I could add it to my command-line parameters.

snuyanzin commented 6 years ago

Yes, exactly. There is a variable of color scheme. The widget changes this value iteratively all over the existing values (including default which means no color scheme). Once it reaches the end of the list it continues with the start again (cycled list). At the same time yes it is possible to specify required color-scheme as a startup argument or via !set colorscheme <scheme name>.

snuyanzin commented 6 years ago

Added a widget with a keyboard combination alt-h (like highlighting). The switch via !set colorscheme <scheme name> is also possible. The startup argument via --colorscheme <scheme name> is also possible

The keyboard combination could be changed however have to take into account there is only a limited set of possible combinations because of lots of bounded jline3 widgets for instance https://github.com/jline/jline3/blob/cd4c5f9d1e6e3a35105a9c17e6755ee5e382d373/reader/src/main/java/org/jline/reader/impl/LineReaderImpl.java#L5361

julianhyde commented 6 years ago

I have reviewed your PR #176:

julianhyde commented 6 years ago

When I tried '!set colorscheme invalid', it accepted it without error, and the next thing I typed (as it happens, up-arrow) caused sqlline to go into a loop, printing

    at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:462)
    at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:404)
    at sqlline.SqlLine.begin(SqlLine.java:537)
    at sqlline.SqlLine.start(SqlLine.java:262)
    at sqlline.SqlLine.main(SqlLine.java:193)

and not accepting any input. Let's fix that before we merge.

snuyanzin commented 6 years ago

Thank you for the feedback and findings. I found that you merged something to scratch branch, that is why I did changes and pushed them to that branch to simplify merge. (#189 )

  • I changed the terminology a little, 'sqlKeywordsStyle' to 'keywordStyle';
  • I broke out "class AttributedStyles" and "enum BuiltInHightlightStyle" to contain some of the constants;
  • I cleaned up some comments.

Yes it becomes better, thank you.

  • SqlLineHighlighter.connection2Rules looks like it might cause a leak of Connection objects. If you agree, how about using a weak map, or putting the rule inside the DatabaseConnection?

Yes I agree about weak map here and use it in PR

  • Do you plan to implement completion? When I type '!set colorscheme ' and press 'tab', it only suggests 'default'

There is the similar issue is with outputformat, boolean properties. After resolution of #187 yes autocompletion will work suggesting only existing color schemes for a command !set colorscheme

*How about renaming 'ozbsidian' to 'obsidian'? I know that your 'ozbidsian' style is based on another 'ozbsidian' style, which is based on another 'obsidian' style, but 'obsidian' is a real English word, not an in-joke, and easier for most people to type.

for me it is ok, so I did renaming in the mentioned PR

julianhyde commented 6 years ago

Did you document the alt-h key-stroke? I couldn't find it.

julianhyde commented 6 years ago

I'm curious why you didn't add it to DatabaseConnection, which seems simpler to me. (Weak map is fine, no need to change it.)

julianhyde commented 6 years ago

I took a stab at removing connection2Rules in e91298e. I assumed that even if a connection has more than one highlighter, then the same rule can be used for all highlighters. The rule is initialized when the first highlighter is set. Because the rule is stored in the DatabaseConnection we don't need any maps. I think this change removes some failure modes. Let me know what you think.

snuyanzin commented 6 years ago

Why did I do it before? Well as far as I remember(it was about a month ago) I wanted to encapsulate everything related to highlighting. However, now I see that the same rule could be used not only for highlight purposes but for instance for line continuation and completion as well. In this case it makes sense to have it on a DatabaseConnection level. Also as you have mentioned such approach would be safer. So, yes now after these points came to my mind I agree on you. Thank you.

The only comments I have to your commit e91298e:

  1. It makes sense to remove private SqlLineHighlighter highlighter; and the line where it is initialized, as now it is not used anymore.
  2. As potentially this rule could be useful not only for highlighting, may be it would make sense to extract it to DatabaseConnection level and rename it to something like ConnectionSpecificRule. If there is no objections I think it could be done while resolution of #187 or #190.
snuyanzin commented 6 years ago

Did you document the alt-h key-stroke? I couldn't find it.

No, I didn't. But what is the place where it could be added? The only thing I could imagine is manual.txt/xml, section related to colorscheme here https://github.com/julianhyde/sqlline/blob/e91298e8afd1321526370a572eca2dae31efa0af/src/docbkx/manual.xml#L2769.

At the same time it would make sense to have a list of available key-strokes with brief descriptions (plenty of new key-strokes appeared after moving to jline3). May be a separate section in the manual which would aggregate such list of key-strokes.

julianhyde commented 6 years ago

If there is no objections I think it could be done while resolution of #187 or #190.

Perfect. The code can evolve the requirements evolve.

julianhyde commented 6 years ago

Maybe a (brief) hot-keys section in the output of !help, similar to the 'Variables:' section. Hyperlink to jline3's built-in keys, and only list sqlline-specific keys like alt-h.

julianhyde commented 6 years ago

Fixed in 9448b38, which was based on PR #176 with a few of my changes.

There are a few follow-up tasks (documenting alt-h, #187 and #190, and auto-completion of the !set colorscheme command) but there's plenty in this fix already and the only major bug is fixed. Thanks @snuyanzin, for the excellent work, as usual!