Skip to content

[Potentially breaking change] changed specs2 to single bind #319

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

Merged
merged 3 commits into from
Oct 23, 2017

Conversation

ittaiz
Copy link
Contributor

@ittaiz ittaiz commented Oct 21, 2017

changed specs2 to single bind to remove volatility of version upgrade (dependency graph changes)
@johnynek following the discussion in #311 I realized my implementation was way off with the multiple binds.
This is a breaking change but I actually stumbled upon this need a few times in the past but I didn't realize the problem until your explanation in #311

@ittaiz
Copy link
Contributor Author

ittaiz commented Oct 21, 2017

@chaoren I needed to change the aspect test for this to pass.
Will the plugin work in the above configuration?
Specs2 dependencies are very volatile

@ittaiz ittaiz changed the title [Breaking change] changed specs2 to single bind [Potentially breaking change] changed specs2 to single bind Oct 21, 2017
@ittaiz
Copy link
Contributor Author

ittaiz commented Oct 21, 2017

Also this will be a breaking change only to users who've customized specs2 usage

@chaoren
Copy link
Contributor

chaoren commented Oct 21, 2017

Yes, it looks fine. It's even better now that the aspect test doesn't need to be changed every single time the dependencies change.

@ittaiz
Copy link
Contributor Author

ittaiz commented Oct 22, 2017 via email

@chaoren
Copy link
Contributor

chaoren commented Oct 22, 2017

It doesn't matter, since scala_specs2_junit_test is just a macro on top of scala_junit_test, all the aspect sees is a scala_junit_test with a bunch of extra deps attached.

@ittaiz
Copy link
Contributor Author

ittaiz commented Oct 22, 2017

@johnynek I want to merge this one and #320 tomorrow since I have a backlog of stuff to upstream and just fix here:

  1. jars2labels accumulation has bugs (push upstream)
  2. Bazel 0.7.0 rules_Scala integration broken (don't know how to fix yet, not just a line change)
  3. IntelliJ break (intellij head and rules_scala head broken as @gregestren shows in Integrate intellij scala aspect integration testing into rules_scala #309 )
  4. scala_import rule (needed by many and is crucial for strict deps)
    Due to our problematic monolith many changes need to be serial or accumulated to a big PR. Between the two I'd rather serialize them and try to have quick review cycles on small PRs.

@ittaiz
Copy link
Contributor Author

ittaiz commented Oct 23, 2017

@or-shachar wdyt?

Copy link
Contributor

@or-shachar or-shachar left a comment

Choose a reason for hiding this comment

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

LGTM. maybe just add the missing newlines at eof

"//external:io_bazel_rules_scala/dependency/scala/scala_library",
"//external:io_bazel_rules_scala/dependency/scala/scala_reflect"
]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

[cosmetic] missing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"//external:io_bazel_rules_scala/dependency/scala/parser_combinators",
"//external:io_bazel_rules_scala/dependency/scala/scala_library",
"//external:io_bazel_rules_scala/dependency/scala/scala_reflect"]
return ["//external:io_bazel_rules_scala/dependency/specs2/specs2"]
Copy link
Contributor

Choose a reason for hiding this comment

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

[cosmetic] - misssing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ittaiz ittaiz merged commit 9171c48 into master Oct 23, 2017
@ittaiz ittaiz deleted the specs2_binding branch April 15, 2018 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants