komsit37 / sublime-q

Sublime Text Plugin for q/kdb
MIT License
24 stars 9 forks source link

First commit of new q syntax from atf1206's q-kdb-syntax #20

Closed atf1206 closed 4 years ago

atf1206 commented 4 years ago

Rework of q syntax file for sublime see https://github.com/atf1206/q-kdb-syntax for readme

komsit37 commented 4 years ago

@atf1206 Thanks for the PR! I don't know much about syntax file so it's good to have a PR from you. I did a comparison and just have a few comments:

  1. Keyword if and symbol ` is now highlighted correctly, so that's great!
  2. Can we also color the operator: $, #, ?, @, ., etc?
  3. I'd love to keep color for \xxx functions too (i.e. \d)
  4. Seems you removed the highlight for comma, and parentheses. Why do you dislike them? :) It's ok to not highlight (). But for ,, because it is a join function, it's clearer to see the color (see line 16)
  5. Also, I would expect the : in line 33 to be highlighted as well (and also line 41 :val)
  6. On line 23, files@:where, the file shouldn't be highlighted?
  7. Your repo also has a syntax file for k and q_output. would be nice to include those as well?

image

atf1206 commented 4 years ago

Hi @Komsit37 thanks for the review.

*2. The main question, I think, is that this version does NOT highlight operators ($, #, ? etc), only highlighting keywords. This is maybe a matter of opinion; I see that coloring is used for similar operators in python, but I am not used to it from (e.g.) c/c++.

*4, 5. This is also why for 4. we do not color commas, nor the colon in 5., since I consider return to be an operator (colons ARE highlighted for assignment and i/o, i.e. 0: 1: 2: ).

*6. The new version also handles compound assignment, so I believe that 6. is being handled correctly (compound assignment with apply).

*3. Since anything after \ is a system command, do you think there should be coloring only for kdb-specific commands (e.g. d, as you suggested), or all commands (e.g. \free)?

*7. It seemed easier to make them a separate PR, especially since they are newer and have not been tested much. But yes I would love to get them included in the future.

komsit37 commented 4 years ago

For 2, 4 - yeah this may be a matter of opinion. I feel some code like Line 15 is more readable with operator color. So unless many people prefer the changes, I'd like to keep it this way.

6 - ok . nice

3 - preferably only valid command, but that would require a hard code list of valid command in the syntax file. so to keep things simpler, just highlight all commands should be ok

7 - sure, sounds good

atf1206 commented 4 years ago

Updated! Sorry about the delay.

Operator color added back in.

komsit37 commented 4 years ago

Cool, thanks!