SQLLINE-239: Add extra confirmation step for destructive commands like e Drop/Delete to avoid accidental executions. #263

swaroopak · 2019-01-16T00:18:53Z

Please discard the previous PR(#249) . Something got messed up and I could not see the changes as expected.

snuyanzin

In general it looks ok, however I mentioned some issues which I think are better to be fixed.
+ Could you please fix indentations according to style in files you have updated

src/main/java/sqlline/BuiltInProperty.java

src/main/java/sqlline/SqlLineOpts.java

src/main/resources/sqlline/SqlLine.properties

src/test/java/sqlline/SqlLineArgsTest.java

src/main/java/sqlline/Commands.java

swaroopak · 2019-01-24T01:01:36Z

@snuyanzin @julianhyde I have tried fixing all the comments. Feel free to be specific in case I am missing something.

dbwong

Teammate of Swaroopa here. Took a brief look at this, not too familiar with the code base but a few questions/comments.

src/main/resources/sqlline/SqlLine.properties

src/main/resources/sqlline/manual.txt

src/main/resources/sqlline/SqlLine.properties

src/test/java/sqlline/SqlLineArgsTest.java

karanmehta93 · 2019-02-01T02:36:51Z

src/main/java/sqlline/Commands.java

+        if (Pattern.compile(pattern).matcher(sql).find()
+              && sqlLine.getOpts().getConfirm()) {
+          String question = sqlLine.loc("really-perform-action");
+          final int userResponse = getUserAnswer(question, 'y', 'n');


Potential answers can be 'Y' and 'N' (uppercase only)

It is not clear why uppercase only, for instance in Linux there could be lowercase answers for similar questions.
May be it makes sense to specify both upper and lowercase

Nothing specific, its just that it introduces a small delay since we need to put a thought for upper case letters. Its a good to have thing, I am fine with either.

src/main/java/sqlline/SqlLineOpts.java

karanmehta93 · 2019-02-01T02:43:01Z

Can we configure it using command line?
How can we set this property grammatically when SqlLine is launched?

snuyanzin · 2019-02-01T07:35:54Z

Yes it is possible for any property like --propertyName=propertyValue
e.g. (Windows)

bin\sqlline --fastConnect=true --verbose=true --isolation=TRANSACTION_READ_COMMITTED

or it also could be specified inside %USER_HOME%\sqlline\sqlline.properties (the similar in Linux) which is read while start. However properties from command line have higher priority.

swaroopak · 2019-02-05T04:54:00Z

Thank you @dbwong @karanmehta93 @snuyanzin @julianhyde for reviewing my PR. Per me, I have fixed the comments.

swaroopak · 2019-02-13T01:12:16Z

@julianhyde @snuyanzin @jongyeol @arina-ielchiieva Can one of you please merge this? Thanks.

snuyanzin · 2019-02-14T12:59:02Z

@swaroopak thank you for your corrections. Now it looks much better.
Unfortunately I'm out of time for the nearest 1-2 months to have closer look.
The thing which could be improved is usage of the same style of indentations in files with tests.

swaroopak · 2019-02-15T01:09:21Z

@snuyanzin Thanks for your reply. I fail to see what you mentioned as indentations in SqlLineArgsTest.java. I opened the file with vim, sublime and IntelliJ. It opens properly indented. Please help me understand.

snuyanzin · 2019-02-22T19:57:05Z

@swaroopak sorry for the delay.
I mean lines 2257, 2266-2270, 2272-2273, 2277-2278, 2291, 2294, 2299, 2313, 2314 where there are 4 more spaces than in most other places in the file like

sqlline/src/test/java/sqlline/SqlLineArgsTest.java

Line 255 in eeac43c

+ "-- a comment\n"

…e Drop/Delete to avoid accidental executions.

swaroopak · 2019-03-02T22:26:33Z

@snuyanzin Thanks for your detailed review about spaces. Is there a template file that can be included in intellij/eclipse? I have manually fixed those spaces for now, but it would be nice to have that which can save someone from missing such indentations.

swaroopak · 2019-03-02T22:36:20Z

@snuyanzin I wonder how changing spaces failed the build? I checked it says some java doc error!?
Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.0.1:javadoc Could you please help me?

masayuki038 · 2019-03-03T01:15:41Z

@swaroopak This problem seems JDK issue.

https://bugs.openjdk.java.net/browse/JDK-8219474
https://issues.apache.org/jira/browse/MJAVADOC-555

I see the same error when mvn clean pakcage sqlline/master on adoptopenjdk/openjdk11.
This workaround works for me.

diff --git a/pom.xml b/pom.xml
index b8ff859..4001e77 100644
--- a/pom.xml
+++ b/pom.xml
@@ -215,6 +215,12 @@
         <configuration>
           <additionalOptions>${maven-javadoc-plugin.additionalOptions}</additionalOptions>
           <show>public</show>
+          <javaApiLinks>
+            <property>
+              <name>api_12</name>
+              <value>https://download.java.net/java/early_access/jdk12/docs/api/</value>
+            </property>
+          </javaApiLinks>
         </configuration>
         <executions>
           <execution>

masayuki038 · 2019-03-03T01:48:21Z

I also confirmed that this change works for me. This one is better.

root@e6c75c575b52:/work/sqlline# git diff
diff --git a/pom.xml b/pom.xml
index b8ff859..5992824 100644
--- a/pom.xml
+++ b/pom.xml
@@ -215,6 +215,7 @@
         <configuration>
           <additionalOptions>${maven-javadoc-plugin.additionalOptions}</additionalOptions>
           <show>public</show>
+          <source>${maven.compiler.source}</source>
         </configuration>
         <executions>
           <execution>

julianhyde · 2019-03-11T05:06:51Z

Merged as c83414f, fixes #239.

swaroopak force-pushed the SQLLINE-239_updated branch from e423768 to d11b695 Compare January 16, 2019 00:32

snuyanzin requested changes Jan 16, 2019

View reviewed changes

swaroopak force-pushed the SQLLINE-239_updated branch from d11b695 to da0ded7 Compare January 16, 2019 21:50

snuyanzin reviewed Jan 17, 2019

View reviewed changes

src/main/java/sqlline/Commands.java Outdated Show resolved Hide resolved

swaroopak force-pushed the SQLLINE-239_updated branch from da0ded7 to 1274581 Compare January 17, 2019 20:35

swaroopak mentioned this pull request Jan 18, 2019

Unable to see instruction to quit or read more when run !manual through sqlline CLI #264

Open

swaroopak force-pushed the SQLLINE-239_updated branch 2 times, most recently from 957d777 to f2b63b9 Compare January 24, 2019 00:56

dbwong reviewed Jan 25, 2019

View reviewed changes

swaroopak force-pushed the SQLLINE-239_updated branch 2 times, most recently from 2af02b2 to e81409d Compare January 30, 2019 19:59

swaroopak mentioned this pull request Jan 30, 2019

Add extra confirmation step for destructive commands like Drop/Delete to avoid accidental executions #239

Closed

karanmehta93 reviewed Feb 1, 2019

View reviewed changes

src/main/java/sqlline/SqlLineOpts.java Outdated Show resolved Hide resolved

swaroopak force-pushed the SQLLINE-239_updated branch from e81409d to eeac43c Compare February 5, 2019 04:51

SQLLINE-239: Add extra confirmation step for destructive commands lik…

9266f0f

…e Drop/Delete to avoid accidental executions.

swaroopak force-pushed the SQLLINE-239_updated branch from eeac43c to 9266f0f Compare March 2, 2019 22:22

masayuki038 mentioned this pull request Mar 9, 2019

Failed to generate javadoc on JDK11 #273

Closed

julianhyde closed this Mar 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQLLINE-239: Add extra confirmation step for destructive commands like e Drop/Delete to avoid accidental executions. #263

SQLLINE-239: Add extra confirmation step for destructive commands like e Drop/Delete to avoid accidental executions. #263

swaroopak commented Jan 16, 2019 •

edited

Loading

snuyanzin left a comment

swaroopak commented Jan 24, 2019

dbwong left a comment

karanmehta93 Feb 1, 2019

snuyanzin Feb 1, 2019

karanmehta93 Feb 1, 2019

karanmehta93 commented Feb 1, 2019

snuyanzin commented Feb 1, 2019 •

edited

Loading

swaroopak commented Feb 5, 2019

swaroopak commented Feb 13, 2019

snuyanzin commented Feb 14, 2019

swaroopak commented Feb 15, 2019 •

edited

Loading

snuyanzin commented Feb 22, 2019

swaroopak commented Mar 2, 2019

swaroopak commented Mar 2, 2019

masayuki038 commented Mar 3, 2019

masayuki038 commented Mar 3, 2019

julianhyde commented Mar 11, 2019

SQLLINE-239: Add extra confirmation step for destructive commands like e Drop/Delete to avoid accidental executions. #263

SQLLINE-239: Add extra confirmation step for destructive commands like e Drop/Delete to avoid accidental executions. #263

Conversation

swaroopak commented Jan 16, 2019 • edited Loading

snuyanzin left a comment

Choose a reason for hiding this comment

swaroopak commented Jan 24, 2019

dbwong left a comment

Choose a reason for hiding this comment

karanmehta93 Feb 1, 2019

Choose a reason for hiding this comment

snuyanzin Feb 1, 2019

Choose a reason for hiding this comment

karanmehta93 Feb 1, 2019

Choose a reason for hiding this comment

karanmehta93 commented Feb 1, 2019

snuyanzin commented Feb 1, 2019 • edited Loading

swaroopak commented Feb 5, 2019

swaroopak commented Feb 13, 2019

snuyanzin commented Feb 14, 2019

swaroopak commented Feb 15, 2019 • edited Loading

snuyanzin commented Feb 22, 2019

swaroopak commented Mar 2, 2019

swaroopak commented Mar 2, 2019

masayuki038 commented Mar 3, 2019

masayuki038 commented Mar 3, 2019

julianhyde commented Mar 11, 2019

swaroopak commented Jan 16, 2019 •

edited

Loading

snuyanzin commented Feb 1, 2019 •

edited

Loading

swaroopak commented Feb 15, 2019 •

edited

Loading