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

Simplify status line plus add detailed and silent status reporters #20

Merged
merged 1 commit into from
May 31, 2017

Conversation

erickpintor
Copy link
Contributor

Closes: #14

Copy link

@marrony marrony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just some questions

Log.status(report())
reporter = {
val r = reportType match {
case Detailed => new DetailedReporter(Stats.reg)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about make the report type construct the reporter? Is it too much OO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, considering the implementation details are all private, It really is a matter of code style at this point.

Polymorphism here would make the type definition a bit verbose:

sealed trait ReportType {
  private[report] def reporter: StatsReporter
}

object ReportType {
  final case object Inline extends ReportType {
    private[report] def reporter: StatsReporter = 
      new InlineReporter()
  }
  
  final case object Detailed extends ReportType {
    private[report] def reporter: StatsReporter = 
      new DetailedReporter(Stats.reg)
  }
  
  final case object Silent extends ReportType {
    private[report] def reporter: StatsReporter = 
      new SilentReporter()
  }
}

Note the private[report]. Since StatsReporter is private, I also have to protect the scope of the reporter method.

Considering ReportType is sealed, which makes pattern matching secure at compile time, a patter match looks simpler, IMO.

But I would argue in favor of polymorphic definition in case of a public reusable API, if we'd have one.

@@ -92,6 +93,17 @@ object CmdArgs {
case "continue" => c.config += OnError(ErrorStrategy.DoNotStop)
}

opt[String]("report-type")
.text("Progress report strategy to use")
.valueName(s"<inline|detailed|silent>")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides the amount of info logged, what is the difference between inline and detailed? As far as I could see, detailed is logging using log4j and inline is logging using a scheduler and Log.status, is it mapped to the console?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline uses Log.status which is a fixed status line at the end of the console output that gets rewritten at every update. It does not append \n so, it doesn't make you console scroll down.

Detailed uses the default log4j metrics reporter. It's quite verbose and it logs one metric per line but, it's useful when you need more information about the process' performance or when your output stream doesn't allow you to move the cursor around.

@erickpintor erickpintor merged commit 9632b27 into master May 31, 2017
@erickpintor erickpintor deleted the simplify-status-line branch May 31, 2017 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants