Add a quiet mode #92

chb0github · 2017-10-16T20:11:09Z

No description provided.

* 'master' of github.com:mybatis/migrations: Changed the default value of ignore_warnings from false to true. #46 #84 #89 #91 Oops. refs #86 Added database specific configuration information to the doc.

chb0github · 2017-10-16T21:11:50Z

src/main/java/org/apache/ibatis/migration/options/Options.java

+  QUIET {
+    @Override
+    public void accept(SelectedOptions options, String arg) {
+      options.setQuiet(true);


Maybe I cold parse this as true or false instead of assuming --quiet means true

h3adache · 2017-10-25T16:58:45Z

src/main/java/org/apache/ibatis/migration/options/Options.java

 public enum Options {
-  PATH, ENVPATH, SCRIPTPATH, DRIVERPATH, HOOKPATH, ENV, FORCE, TRACE, HELP, TEMPLATE, IDPATTERN
+


I use to have this like this if you look at the history but I split it out because I didn't want the Options enum to know about SelectedOptions etc. This is an implementation detail and splitting it out was to allow changing the implementation to something else such as using a cmd line parsing library instead.

I see -

Options don't need to know about "Selected" - put selected Options in a Set<Option> selected and have each option apply itself when invoked. We can iterate on that a bit if you want

h3adache · 2017-10-25T17:01:03Z

src/main/java/org/apache/ibatis/migration/commands/BaseCommand.java

+        @Override
+        public void write(int b) throws IOException {
+          // throw away output
+        }


There's a setter for the printstream. This should use that but also it's also interesting that this shows a design flaw that we could set the printstream and override the quiet option without knowing it.

Sure, there is a setter but setters are for outside classes. This is in the constructor - why would I call one of my own public functions?

I now see what you're saying. setPrintStream and quiet are contradictory. I mean, technically you could have quiet doing something really shamless like putting if statements everywhere if(!option.isQuiet()) printStream.println(...) But that leave open a lot of places to make mistakes and just mis it. Plus, if it's quiet (even with these if statements, what's the point of setting the print stream?

The best thing to do is make them mutually exclusive and throw an error if both are set.

This could be done by declaring a specific instance of the quiet stream as:

private static final PrintStream QUIET = new PrintStream(new OutputStream() { @Override public void write(int b) throws IOException { // throw away output } });

and then doing something like:

public void setPrintStream(PrintStream newStream) { if(this.printStream == QUIET) throw new UnsupportedOpertionException("You can't have a quiet output and a set the output stream"); }

h3adache · 2017-10-25T17:02:22Z

src/main/java/org/apache/ibatis/migration/options/Options.java

+      options.setQuiet(true);
+    }
+  };
+  public abstract void accept(SelectedOptions options, String arg);


This also happens. Not everything needs an arg.

True - with strongly typed languages you can't do like javascript does and just get magical arguments trailing on the end. with JS everything is a vararg and you use as many args as you want. Kotlin has a nice feature whereby you can default values. Java isn't like that :(

We can create an overload that does

public void accept(SelectedOptions option) { accept(options,null); }

No big deal

h3adache · 2017-10-25T17:04:08Z

src/main/java/org/apache/ibatis/migration/options/OptionsParser.java

-          options.setIdPattern(argParts[1]);
-          break;
-      }
+      option.accept(options, argParts.length > 1 ? argParts[1] : null);


Probably my fault but the logic is flawed in that it looks at = --foo=true with a shortcut of -f true breaks this.

How is this any different then what was there before? All of these, if they use the argument use argParts[1] if anything the original code was flawed and would allow for an ArrayIndexOutOfBoundsException if you had an arg like --foo=

It looks like, if anything the regex could be bit more robust: "-([a-z])|--([a-z]+)[ =](.*)" then use a Pattern matcher instead of a split and extract the groups to match.

Better yet would be to put that logic in the Option class with a factory method like: Option.of(arg) so you could have something like:

Set<Option> selected = Arrays.stream(args).map(Option::of).collect(toSet());

How is this any different then what was there before?

It's not, I'm pointing out a logic flaw that I missed a while back.
The regex fix is a good idea. (although your [ =] breaks on multiple spaces without a trim).
I don't want to get cute or complicated w a fix here. There's already plans to investigate options parsing library which better handles these. Although, to be fair we don't have a ticket so you wouldn't have known that.

To be honest: Spring boot/spring cli does a fantastic job of this.

-([a-z])|--([a-z]+)[ ]+?=[ ]+?(.*)]
how's that?

any number of spaces before or after the "=" actually, now we are just talking about properties and you're just better off doing something simple like this and passing it to properties:

args = Arrays.stream(args).filter( s -> s.startsWith("-")).map(a -> a.subString(1)); args = Arrays.stream(args).filter( s -> s.startsWith("-")).map(a -> a.subString(1)); Arrays.stream(args).map(a -> String.format("%s%n",a)) .reduce(String::concat).map(String::toByteArray) .map(ByteArrayInputStream::new).map(bais -> { Properties p = new Properties(); p.loadFrom(bais); return p; }.ifPresent(props -> ... apply options);

I wanted to get smarter with a lambda recursion but you get the point.

Parsing properties is trickier than it seems - hence the reason to simply package it up and hand it off to Properties -- don't I wish that class would take an InputStream constructor

h3adache · 2017-10-25T17:05:52Z

src/main/java/org/apache/ibatis/migration/options/SelectedOptions.java

@@ -15,6 +15,7 @@
 */
 package org.apache.ibatis.migration.options;

+// This could just be a Set<Option> selectedOptions = new EnumSet<Option>(Option.class);


If Options were a binary thing then yes but unfortunately they are not. Although having them be strategies to get the option value might be interesting.

can I have and option more than once? Then a Set is perfect.

This class is literally a collection of properties. The properties are either: set (1) or not set (0). That's the definition of binary. And before you ask: null is a set value of null

Imagine an option for each of these properties and each option were a consumer

Option quiet = new QuietOption(); Set<Option> options = new HashSet<>(); options.add(quiet); quiet.apply(thisApp);

public class QuietOption implements Option { public void apply(ThisApp app) { app.setOutputStream(new OutputStream() { void write(byte thing){}}); } }

As you can see: With the other changes I made you now have to APPLY the options separately. The data and the functionality that applies to it should go hand-in-hand ala OOP

Your second comment is exactly what I said about options being strategies. Sorry if that wasn't clear. It's also shows what I meant about options not being binary things. The option itself yes, the values they potentially represent no.

All that means is it's a List<Option> then. But if you decide to go with a library for this then you'll end up doing whatever the library says

h3adache · 2017-10-25T17:07:19Z

This is a nice to have option @chb0github. Thanks for the PR. We can talk about the implementation

chb0github · 2017-10-25T18:25:47Z

@h3adache Ok, let's talk. Which is the part you would do differently?: the use of an anonymous inner class to intercept the output stream or ditching the switch case statement for an abstract enum?

This is very similar to the command pattern which I have outlined a case for
here if you follow the thread you will see how it all evolves

If you have a gitter channel I am happy to engage there

h3adache · 2017-10-26T14:37:02Z

@chb0github the addition of the quiet feature is fine, I think it doesn't have to be bundled into the change of the entire options setup. That's a completely separate issue. I think that we should decide if it's worth safe guarding/checking against quiet option if someone invokes the setter as I mentioned before.
I would rather you simplify this to just that discussion and we can finalize that and apply the change.

chb0github · 2017-10-26T17:02:22Z

Ok, pulling out the refactor I can get on-board with.

As far as the safe-guarding thing: That I don't get. I was just following the pattern.

chb0github · 2017-10-26T17:21:35Z

@h3adache changes are made.

h3adache · 2017-10-26T18:04:01Z

src/main/java/org/apache/ibatis/migration/options/SelectedOptions.java

@@ -15,6 +15,7 @@
 */
 package org.apache.ibatis.migration.options;

+// This could just be a Set<Option> selectedOptions = new EnumSet<Option>(Option.class);


Can you remove this plz. It's not relevant to this issue.

h3adache · 2017-10-26T18:05:41Z

src/main/java/org/apache/ibatis/migration/commands/BaseCommand.java

+
+      this.printStream = new PrintStream(new OutputStream() {
+        @Override
+        public void write(int b) throws IOException {


public void write(int b) {} we aren't doing anything here that throws an exception

Hmm... that's a checked exception and Intellij put this method in for me. I did remove it and it didn't complain.. odd

chb0github · 2017-10-26T21:59:47Z

I saw somewhere someone mentions the target VM was 1.6? I am looking at mybatis-3, particularly this file and it uses a Java 8 feature.

h3adache · 2017-10-27T00:49:16Z

I merged this in. I added --quiet to usages and a warning if printstream is being set as a workaround for the quiet issue we discussed. Thanks for the PR!

Christian Bongiorno and others added 2 commits October 16, 2017 13:08

Add a --quiet flag to supress output except errors

6aed404

Merge branch 'master' of github.com:mybatis/migrations

1751f54

* 'master' of github.com:mybatis/migrations: Changed the default value of ignore_warnings from false to true. #46 #84 #89 #91 Oops. refs #86 Added database specific configuration information to the doc.

chb0github commented Oct 16, 2017

View reviewed changes

chb0github mentioned this pull request Oct 18, 2017

Feature: Quiet mode #97

Closed

h3adache reviewed Oct 25, 2017

View reviewed changes

Remove enum refactor

2831813

h3adache reviewed Oct 26, 2017

View reviewed changes

Removed stuff per CR

0da9e0e

h3adache merged commit 0da9e0e into mybatis:master Oct 27, 2017

harawata changed the title ~~Add a quiet mode and refactor options~~ Add a quiet mode Dec 25, 2017

harawata assigned h3adache Dec 25, 2017

harawata added the enhancement label Dec 25, 2017

harawata added this to the 3.3.2 milestone Dec 25, 2017

harawata added a commit that referenced this pull request Dec 25, 2017

A test case and documentation for #92

1063534

		public enum Options {
		PATH, ENVPATH, SCRIPTPATH, DRIVERPATH, HOOKPATH, ENV, FORCE, TRACE, HELP, TEMPLATE, IDPATTERN

Add a quiet mode #92

Add a quiet mode #92

Uh oh!

Conversation

chb0github commented Oct 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h3adache commented Oct 25, 2017

Uh oh!

chb0github commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h3adache commented Oct 26, 2017

Uh oh!

chb0github commented Oct 26, 2017

Uh oh!

chb0github commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chb0github commented Oct 26, 2017

Uh oh!

h3adache commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

chb0github commented Oct 25, 2017 •

edited

Loading

h3adache commented Oct 27, 2017 •

edited

Loading