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

Added `NonEmptyLazyList` to replace `NonEmptyStream` #2941

Merged
merged 40 commits into from Jul 19, 2019

Conversation

@kailuowang
Copy link
Member

commented Jul 12, 2019

The NonEmptyStream based on OneAnd cannot be simply modified to wrap LazyList since the OneAnd no longer provides stack safety for LazyList. Thus I switched to use the newtype based approach.
I also refactor NonEmptyChain (also newtype based) to reduce code duplication.

This extracted from #2930 hence the long history, will squash merge as usual

kailuowang and others added 30 commits Jun 21, 2019
private[data] trait Tag extends Any
type Type[+A] <: Base with Tag

private[cats] def create[A](s: LazyList[A]): Type[A] =

This comment has been minimized.

Copy link
@travisbrown

travisbrown Jul 12, 2019

Collaborator

Do you happen to know if these aren't private[data] just because of the tests? It seems like e.g. NonEmptyStreamSuite should be in cats.data, not cats.tests.

This comment has been minimized.

Copy link
@kailuowang

kailuowang Jul 12, 2019

Author Member

Right now all tests suites are under cats.tests, do we want to make it more conventional (i.e. parallel package tree as in src)?

This comment has been minimized.

Copy link
@travisbrown

travisbrown Jul 12, 2019

Collaborator

I'd say yes, especially if it's the only reason these aren't more restricted. That's pretty far from the topic of this PR, though. I can take a look this weekend.

This comment has been minimized.

Copy link
@kailuowang

kailuowang Jul 12, 2019

Author Member

just found out that the tests don't use them. So it's probably a simple artifact of copy paste code. I just changed all of such methods to be private[data]

This comment has been minimized.

Copy link
@travisbrown

travisbrown Jul 12, 2019

Collaborator

Nice, thanks!

@codecov-io

This comment has been minimized.

Copy link

commented Jul 12, 2019

Codecov Report

Merging #2941 into master will decrease coverage by 0.11%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2941      +/-   ##
==========================================
- Coverage   94.03%   93.91%   -0.12%     
==========================================
  Files         370      371       +1     
  Lines        7038     7036       -2     
  Branches      186      176      -10     
==========================================
- Hits         6618     6608      -10     
- Misses        420      428       +8
Impacted Files Coverage Δ
...ore/src/main/scala/cats/data/NonEmptyMapImpl.scala 96.15% <ø> (ø) ⬆️
core/src/main/scala/cats/data/NonEmptyList.scala 100% <ø> (ø) ⬆️
.../scala/cats/kernel/instances/StreamInstances.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/package.scala 90.9% <ø> (ø) ⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/NonEmptySet.scala 97.61% <ø> (ø) ⬆️
...rc/main/scala/cats/laws/discipline/arbitrary.scala 99.15% <ø> (ø)
...in/scala/cats/data/AbstractNonEmptyInstances.scala 100% <100%> (ø)
core/src/main/scala/cats/data/NonEmptyChain.scala 82.71% <87.5%> (-14.61%) ⬇️
... and 2 more

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 07b2c21...4c0febb. Read the comment docs.

kailuowang added 2 commits Jul 12, 2019

@kailuowang kailuowang added this to the 2.0.0-RC1 milestone Jul 12, 2019

@LukaJCB LukaJCB requested review from fthomas, rossabaker and johnynek Jul 16, 2019

object NonEmptyLazyList extends NonEmptyLazyListInstances {
private[data] type Base
private[data] trait Tag extends Any
type Type[+A] <: Base with Tag

This comment has been minimized.

Copy link
@johnynek

johnynek Jul 16, 2019

Collaborator

I don’t understand how these are abstract on a concrete object. What is Type?

This comment has been minimized.

Copy link
@kailuowang

kailuowang Jul 16, 2019

Author Member

This is the newtype encoding (kind of a hack of the Scala type system) that eliminates all runtime overhead. I didn't come up with it. But it has been battle tested cats since 1.0 in NonEmtySet and NonEmptyMap, and later NonEmptyChain

This comment has been minimized.

Copy link
@johnynek

johnynek Jul 16, 2019

Collaborator

I know the trick, I just didn't know objects could have totally abstract types. Is that intended, or some quirk of the compiler?

Seems to me, anything we an abstract type should have to be abstract.

Do we have this documented somewhere that we can link to? I think it is not at all clear to the reader what the various parts are here and why they are all required:

  1. why do we need Base?
  2. why do we need Tag?
  3. why should the above be private[data] vs private or public?
  4. how is the aliasing of this type to NonEmptyLazyList accomplished? Is that a package alias?

This comment has been minimized.

Copy link
@kailuowang

kailuowang Jul 16, 2019

Author Member

good point. Link added.

This comment has been minimized.

Copy link
@kailuowang

kailuowang Jul 16, 2019

Author Member

also cc @LukaJCB to see if he has anything to add to the simple description and the link


def range(start: Long, endInclusive: Long): NonEmptyLazyList[Long] = {
// if we inline this we get a bewildering implicit numeric widening
// error message in Scala 2.10

This comment has been minimized.

Copy link
@johnynek

johnynek Jul 16, 2019

Collaborator

Since we don’t use 2.10 anymore can we remove?


def range(start: Long, endInclusive: Long): NonEmptyChain[Long] = {
// if we inline this we get a bewildering implicit numeric widening
// error message in Scala 2.10

This comment has been minimized.

Copy link
@johnynek

johnynek Jul 16, 2019

Collaborator

Can we remove?

kailuowang added 4 commits Jul 16, 2019
@kailuowang

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

@johnynek did my changes address the issues?

@johnynek
Copy link
Collaborator

left a comment

sorry for the latency

@kailuowang

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

@LukaJCB, do you want to review the new changes?

@LukaJCB
Copy link
Member

left a comment

Sorry for the delay, this looks good to go! :)

@LukaJCB LukaJCB merged commit 7e94169 into typelevel:master Jul 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
kailuowang added a commit that referenced this pull request Jul 20, 2019
add back mistakenly removed overrides
These performance overrides were mistakenly removed by #2941
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.