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

Give higher priority to partial-unification fix #1946

Merged
merged 2 commits into from
Oct 3, 2017

Conversation

LukaJCB
Copy link
Member

@LukaJCB LukaJCB commented Oct 3, 2017

After answering questions about relating to partial-unification, I think we should reprioritize its standing in the Readme.md. This PR mentions the fix up front instead of a side-note further down below. WDYT? :)

@codecov-io
Copy link

codecov-io commented Oct 3, 2017

Codecov Report

Merging #1946 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1946   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files         248      248           
  Lines        4454     4454           
  Branches      117      117           
=======================================
  Hits         4257     4257           
  Misses        197      197

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 0997f43...49dd777. Read the comment docs.

kailuowang
kailuowang previously approved these changes Oct 3, 2017
@tpolecat
Copy link
Member

tpolecat commented Oct 3, 2017

This is an improvement but I think I would probably still miss it ... when I look at a README like this I tend to gloss over everything other than the code blocks. Don't know if anyone else does this. I think I would be more likely to notice something like

Cats relies on improved type inference via the fix for SI-2112, which is not enabled by default. For Scala 2.11.9 or later you should add the following to your build:

scalacOptions += "-Ypartial-unification"

Or, if you need to support older versions of Scala you can use the sbt-partial-unification plugin which extends support back through Scala 2.10.6 or later:

addSbtPlugin("org.lyranthe.sbt" % "partial-unification" % "1.1.0")

@kailuowang
Copy link
Contributor

Good idea @tpolecat

@LukaJCB
Copy link
Member Author

LukaJCB commented Oct 3, 2017

Thanks @tpolecat, I definitely agree with your suggestion! :)
I added something along the lines, WDYT?

Copy link
Member

@tpolecat tpolecat left a comment

Choose a reason for hiding this comment

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

👍

@fthomas fthomas merged commit 7622a90 into master Oct 3, 2017
@kailuowang kailuowang deleted the LukaJCB-prioritize-partial-unification branch October 3, 2017 20:59
@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants