SQL: various improvements and fix if/case/loop/end detection with SQL (Oracle)
Needs ReviewPublic

Authored by jpoelen on Mar 18 2018, 10:27 PM.

Details

Reviewers
dhaumann
Group Reviewers
Framework: Syntax Highlighting
Summary

Previously, the variables prefixed by if / case / loop / end are a mix of keywords and normal text.

Improvements:

  • Categorization of "Keywords" to "Operator Keyword" and "ControlFlow".
  • Removes duplicate keywords in "functions" and "keywords" list.
  • Some elements of the keywords list are moved to the functions list.
  • Adds SQL functions.
  • Removes symbols from Keyword list.

Diff Detail

Repository
R216 Syntax Highlighting
Branch
sql2
Lint
No Linters Available
Unit
No Unit Test Coverage
jpoelen created this revision.Mar 18 2018, 10:27 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 18 2018, 10:27 PM
jpoelen requested review of this revision.Mar 18 2018, 10:27 PM
jpoelen updated this revision to Diff 29866.Mar 18 2018, 10:33 PM
This comment was removed by jpoelen.

There is a bug on which I fell while completing the tests: the output is not stable.

Several successive calls to ./bin/kate-syntax-highlighter $PROJ/autotests/input/test.sql give a different result. valgrind returns a lot of "Conditional jump or move depends on uninitialised value(s) ", but I do not have the debug symbols and I do not know which library the error comes from (I tried with qt5-debug-base without result).

dhaumann requested changes to this revision.Apr 14 2018, 12:46 AM

Looks mostly good, but there are some minor issues we first need to address:

  • dsControlFlow and similar default styles were added with KDE Frameworks 5, so kateversion="5.0" is required in the header
  • WordDetect for things like "end if" is not sensitive to multiple spaces anymore. Are you sure this is correct? If not, please keep RegExpr.

Please provide an updated revision, I think the next one is good to go in. :-)

data/syntax/sql-mysql.xml
9

kateversion="5.0" is required due to dsControlFlow.

data/syntax/sql-oracle.xml
4

Since you are using dsControlFlow and similar new itemDatas, you have to change kateversion="5.0".

1873

Hm, I can see that WordDetect is possibly faster than the RegExpr here. Still, I *assume* there are arbitrary many spaces allowed in "end if". If (and only if) this is true, then I would prefer to keep the RegExpr variant. Same for the others.

The changes from StringDetect to WordDetect are certainly fine.

data/syntax/sql-postgresql.xml
6

kateversion="5.0" :-)

data/syntax/sql.xml
8

kateversion="5.0"

This revision now requires changes to proceed.Apr 14 2018, 12:46 AM
jpoelen updated this revision to Diff 32852.Apr 23 2018, 12:28 AM
  • SQL: various improvements and fix if/case/loop/end detection with SQL (Oracle)
  • add sql tests
  • update kateversion to 5.0 and allows multiple spaces for "end if", "end case" and "end loop"
jpoelen updated this revision to Diff 32853.Apr 23 2018, 12:37 AM
  • autotests/input/highlight.sh.syntax and autotests/input/highlight.sh are restored
jpoelen updated this revision to Diff 32855.Apr 23 2018, 12:57 AM
  • diff with the proper branch
jpoelen updated this revision to Diff 32860.Apr 23 2018, 1:11 AM
  • SQL: various improvements and fix if/case/loop/end detection with SQL (Oracle)
  • add sql tests
  • update kateversion to 5.0 and allows multiple spaces for "end if", "end case" and "end loop"
  • autotests/input/highlight.sh.syntax and autotests/input/highlight.sh are restored
jpoelen updated this revision to Diff 32861.Apr 23 2018, 1:35 AM

I am completely lost with arc diff. The command adds files that I did not normally modify in this branch and specifiying a reference commit does not seem to work.

I ended up doing everything through a new branch, sorry to rot the history like this :/.

rkflx added a subscriber: rkflx.Apr 23 2018, 11:14 AM

Sorry for chiming in, but in light of the focus goal T7116: Streamlined onboarding of new contributors it's important to fix rough edges in our infrastructure, too:

I am completely lost with arc diff. The command adds files that I did not normally modify in this branch and specifiying a reference commit does not seem to work.

Do you know about our recommended workflow in https://community.kde.org/Infrastructure/Phabricator#Workflow? Happy to help with that, just let me know.

I ended up doing everything through a new branch, sorry to rot the history like this :/.

Phabricator's Differential is simply a list of patch iterations if you will, there is no "history" as such which you might have impacted. Only if you use the web uploader instead of Arcanist you make life for the reviewer a bit harder (no author information when landing, no context in the diff view, no base commit).

The page on Phabricator is much better than before. My problem was about creating a branch without the -t option which requires me to specify a commit/branch to do the diff.

I also realized later that I was reloading a page from the revision history, so I always saw the same bad file list and did not understand why. In fact, the last 3 diff are exactly the same.