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

Cleanup VersionNumberSpec #221

Merged
merged 3 commits into from
Mar 20, 2018
Merged

Cleanup VersionNumberSpec #221

merged 3 commits into from
Mar 20, 2018

Conversation

dwijnand
Copy link
Member

No description provided.

class VersionNumberSpec extends UnitSpec with Inside {
import VersionNumber.{ SemVer, SecondSegment }

version("1") { implicit v =>
Copy link
Member

Choose a reason for hiding this comment

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

The overall looks is cleaner, but I am not really sure if I like implicit used just so it doesn't pass in one parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can change that implementation detail

Copy link
Member

Choose a reason for hiding this comment

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

Another sort of meta question is: Do we make custom DSL for each spec / unit tests we write that reads like English statements? I'd much rather prefer we used something like JUnit or Minitest, if we are moving away from plain UnitSpec.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to discuss and/or experiment with alternatives to UnitSpec.

but note that I'm not moving away from UnitSpec in this PR.

I am in favour of creating helper methods to remove lots of repetition and improve legibility (and maintainability), but the fact that it reads like English statements is not my objective.

I'm not sure where the line is drawn between creating a few helper methods and removing repetition becomes a "custom DSL".

Copy link
Member

Choose a reason for hiding this comment

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

"1" should "be parsed" in beParsedAs("1", Seq(1), Seq(), Seq())
it should "breakdown" in breakDownTo("1", Some(1))

In the above, beParsedAs is an English style helper function idiomatic to ScalaTest's FlatSpec.

but note that I'm not moving away from UnitSpec in this PR.

I'd say you've made our own SomethingSpec that internally uses FlatSpec. I really like it, but I also want to stick to an existing test framework, esp if we want others to follow it. Perhaps the style we should migrate is FunSuite:

  test("1") {
    val v = "1"
    assertParsesTo(v, Seq(1), Seq(), Seq())
    assertBreaksDownTo(v, Some(1))
    assertCascadesTo(v, Seq("1"))
  }

Less noise. WDYT?

@dwijnand
Copy link
Member Author

@eed3si9n I went with FreeSpec because FunSuite doesn't support nesting.

wdyt? I think the "assert" prefix is just noise, but I can live with it.

Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

LGTM once it passes Travis

@dwijnand dwijnand merged commit c6b2b62 into sbt:1.x Mar 20, 2018
@dwijnand dwijnand deleted the cleanup/VersionNumberSpec branch March 20, 2018 09:36
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