Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a quiet mode #92

Merged
merged 4 commits into from Oct 27, 2017

Conversation

Projects
None yet
3 participants
@chb0github
Copy link
Contributor

chb0github commented Oct 16, 2017

No description provided.

Christian Bongiorno and others added some commits Oct 16, 2017

Merge branch 'master' of github.com:mybatis/migrations
* '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.
QUIET {
@Override
public void accept(SelectedOptions options, String arg) {
options.setQuiet(true);

This comment has been minimized.

@chb0github

chb0github Oct 16, 2017

Author Contributor

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

@chb0github chb0github referenced this pull request Oct 18, 2017

Closed

Feature: Quiet mode #97

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

This comment has been minimized.

@h3adache

h3adache Oct 25, 2017

Member

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.

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

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

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

This comment has been minimized.

@h3adache

h3adache Oct 25, 2017

Member

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.

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

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?

This comment has been minimized.

@chb0github

chb0github Oct 26, 2017

Author Contributor

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");

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

This comment has been minimized.

@h3adache

h3adache Oct 25, 2017

Member

This also happens. Not everything needs an arg.

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

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

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

This comment has been minimized.

@h3adache

h3adache Oct 25, 2017

Member

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

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

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());

This comment has been minimized.

@h3adache

h3adache Oct 25, 2017

Member
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.

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

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

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

-([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

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

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

This comment has been minimized.

@h3adache

h3adache Oct 25, 2017

Member

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.

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

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

This comment has been minimized.

@h3adache

h3adache Oct 25, 2017

Member

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.

This comment has been minimized.

@chb0github

chb0github Oct 25, 2017

Author Contributor

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

This comment has been minimized.

Copy link
Member

h3adache commented Oct 25, 2017

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

@chb0github

This comment has been minimized.

Copy link
Contributor Author

chb0github commented Oct 25, 2017

@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

This comment has been minimized.

Copy link
Member

h3adache commented Oct 26, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

chb0github commented Oct 26, 2017

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

This comment has been minimized.

Copy link
Contributor Author

chb0github commented Oct 26, 2017

@h3adache changes are made.

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

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

This comment has been minimized.

@h3adache

h3adache Oct 26, 2017

Member

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


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

This comment has been minimized.

@h3adache

h3adache Oct 26, 2017

Member

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

This comment has been minimized.

@chb0github

chb0github Oct 26, 2017

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

chb0github commented Oct 26, 2017

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 h3adache merged commit 0da9e0e into mybatis:master Oct 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@h3adache

This comment has been minimized.

Copy link
Member

h3adache commented Oct 27, 2017

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!

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

@harawata harawata added this to the 3.3.2 milestone Dec 25, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.