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

Move to SBT Pack instead of Assembly #78

Merged
merged 5 commits into from Apr 18, 2018
Merged

Move to SBT Pack instead of Assembly #78

merged 5 commits into from Apr 18, 2018

Conversation

idreeskhan
Copy link
Contributor

@idreeskhan idreeskhan commented Apr 16, 2018

#78
#77
#76
and potentially #67

We want to be able to move to SBT Pack instead of assembly to simplify the dependency management and avoid conflicts with building the fat jar. To make the usage easier, we reduce to a single main so that there is only one entry point, and those who brew install can still use ratatool [args] to run commands

Should keep spotify/homebrew-public#24 in sync and merge that PR when this one is merged

@codecov-io
Copy link

codecov-io commented Apr 16, 2018

Codecov Report

Merging #78 into master will decrease coverage by 0.56%.
The diff coverage is 16.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage   70.11%   69.55%   -0.57%     
==========================================
  Files          22       22              
  Lines         947      959      +12     
  Branches      127      127              
==========================================
+ Hits          664      667       +3     
- Misses        283      292       +9
Impacted Files Coverage Δ
...ain/scala/com/spotify/ratatool/tool/Ratatool.scala 0% <0%> (ø)
...om/spotify/ratatool/tool/DirectSamplerParser.scala 97.87% <100%> (ø)
...ala/com/spotify/ratatool/samplers/BigSampler.scala 63.94% <33.33%> (-0.14%) ⬇️
...in/scala/com/spotify/ratatool/diffy/BigDiffy.scala 46.77% <33.33%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6028d7...71a0119. Read the comment docs.

@@ -174,7 +160,6 @@ lazy val ratatoolCli = project
name := "ratatool-cli",
libraryDependencies ++= Seq(
"com.github.scopt" %% "scopt" % scoptVersion,
"org.apache.parquet" % "parquet-avro" % parquetVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

did we mean to remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I think it ended up there as an artifact during the project restructure but seems to work fine without

| Usage: ratatool [bigDiffy|bigSampler|directSampler] [args]
""".stripMargin

if (args.isEmpty || !Set("bigDiffy", "bigSampler", "directSampler").contains(args.head)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make these variables constants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

//scalastyle:off cyclomatic.complexity
def main(args: Array[String]): Unit = {
val usage = """
| Ratatool - a tool for random data generation and sampling
Copy link
Contributor

Choose a reason for hiding this comment

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

what about diffing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch

}

val o = opts.get
o.mode match {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of get could we do opts.foreach

Copy link
Contributor

@jbigred1 jbigred1 left a comment

Choose a reason for hiding this comment

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

This looks really cool nicely done

Do we need to update any documentation

@idreeskhan
Copy link
Contributor Author

Yes definitely need to update docs, will roll that into this PR as well

@idreeskhan idreeskhan merged commit 0cc0311 into master Apr 18, 2018
@idreeskhan idreeskhan deleted the idrees/sbt-pack branch April 18, 2018 15:30
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

3 participants