[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Github user arina-ielchiieva commented on the issue: https://github.com/apache/drill/pull/1043 I will but it's high time to learn how to do it :) ---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Github user kkhatua commented on the issue: https://github.com/apache/drill/pull/1043 May be @arina-ielchiieva can help resolve squashing into one commit. LGTM ... thanks for doing this! +1 ---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Github user cgivre commented on the issue: https://github.com/apache/drill/pull/1043 I made the requested fixes, and attempted to squash the commits into one, but failed on that last step. I actually renamed the js director `ace-code-editor`, which seemed descriptive to me. ---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Github user kkhatua commented on the issue: https://github.com/apache/drill/pull/1043 @cgivre Just edited the last comment to remove the +1 [ Learnt that I should wait till there is a response to my asks first! :) ] . To summarize, here are my 3 asks: 1. Change the theme to **Crimson** or **Eclipse** since that's a more familiar theme and actually helps the colors stand out. 2. Change the `src-min-noconflict` directory to reflect the name of the library, whose files it contains. e.g. `aceJs` 3. Remove any non-mandatory files, e.g. `snippets` directory doesn't seems to be a requirement for the library. This would help in maintaining future updates to the library or debugging. ---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Github user kkhatua commented on the issue: https://github.com/apache/drill/pull/1043 Oh.. just for a heads up (I guess you already knew this), I zeroed in on the 2 themes based on copy-pasting a sample storage plugin into this: https://ace.c9.io/build/kitchen-sink.html ---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Github user kkhatua commented on the issue: https://github.com/apache/drill/pull/1043 Very nice, @cgivre ! I've been looking to do something similar for the SQL side as well, but with autocomplete support. Let's talk about that separately. We just need to make sure that there are no license/usage issues. Would be nice if we can leverage this into the SQL Editor as well. Let's talk over this offline. I know I had a discussion recently with someone regarding validation of the JSON for the storage plugin, but that will be a stretch. Also, it seems like the library can recognize single line comments ( // ), which (I believe) is supported by Drill JSON config parser. Can you pick a theme that helps visibly pop out the colors more than it currently is? Crimson or Eclipse themes look better, helping visualize. Also, if the 'src-min-noconflict' is primarily to support the ace libraries and you don't want to risk renaming the library files, it's good to give it a more meaningful name (indicating that it contains AceJS libraries). Otherwise, LGTM +1 ---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Github user arina-ielchiieva commented on the issue: https://github.com/apache/drill/pull/1043 @cgivre looks really nice, thanks for making the change! @kkhatua could you please take a look at this PR? ---
