You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by cgivre <gi...@git.apache.org> on 2017/11/20 21:08:53 UTC
[GitHub] drill pull request #1043: DRILL-5981: Add Syntax Highlighting and Error Chec...
GitHub user cgivre opened a pull request:
https://github.com/apache/drill/pull/1043
DRILL-5981: Add Syntax Highlighting and Error Checking to Storage Plugin Config
This PR adds syntax highlighting to the storage plugin configuration page in the Drill web UI, as shown below.
In addition, and the real reason I'd like to see this included is that it also incorporates error checking, again as shown below:
I know there is some objection about including javascript libraries in Drill. However, I work on a closed network and it is difficult if not impossible to build Drill from source if the libraries are not included. I removed all JavaScript files except the bare minimum.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cgivre/drill syntax-highlighting-storage-plugin
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1043.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1043
----
commit 8e1d52a3d9b08d789574e213fd07721e5b02cedf
Author: cgivre <cg...@gmail.com>
Date: 2017-11-20T20:43:23Z
Initial Commit
Final Commits
Adds syntax highlighting and error checking to plugin configuration page.
Removed extraneous files
----
---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Posted by kkhatua <gi...@git.apache.org>.
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...
Posted by kkhatua <gi...@git.apache.org>.
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...
Posted by cgivre <gi...@git.apache.org>.
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...
Posted by arina-ielchiieva <gi...@git.apache.org>.
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...
Posted by arina-ielchiieva <gi...@git.apache.org>.
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?
---
[GitHub] drill pull request #1043: DRILL-5981: Add Syntax Highlighting and Error Chec...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1043
---
[GitHub] drill issue #1043: DRILL-5981: Add Syntax Highlighting and Error Checking to...
Posted by kkhatua <gi...@git.apache.org>.
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...
Posted by kkhatua <gi...@git.apache.org>.
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.
---