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

Make NonEmptyList covariant #1424

Merged
merged 1 commit into from
Jan 6, 2017

Conversation

edmundnoble
Copy link
Contributor

In which NonEmptyList and ValidationNel are made covariant.

@codecov-io
Copy link

codecov-io commented Oct 25, 2016

Current coverage is 92.40% (diff: 100%)

Merging #1424 into master will not change coverage

@@             master      #1424   diff @@
==========================================
  Files           246        246          
  Lines          3739       3739          
  Methods        3618       3617     -1   
  Messages          0          0          
  Branches        121        122     +1   
==========================================
  Hits           3455       3455          
  Misses          284        284          
  Partials          0          0          

Powered by Codecov. Last update 468e753...a38a588

@johnynek
Copy link
Contributor

johnynek commented Nov 6, 2016

I'm +1 on this. I don't really buy the variance anxiety.

That said, you can always do .widen[T] to use Functor to get it to the type you want, which should cast, which should basically be free.

I think the cats position on variance is something that deserves a doc. I actually enjoy variance, but that seems to not be a popular opinion. I know subtyping in general is not widely admired due to how it complicates type inference, but I don't know of the concrete problems we have with covariance on data types, and I'd like to see them.

@edmundnoble
Copy link
Contributor Author

I believe attaching variance to data types and not typeclasses is the current cats position; the thing is, variance composes much more easily than functors. Once you get more than one level deep, Functor[F].compose(Functor[G]).widen(t) starts to get much worse than just t.

@ceedubs
Copy link
Contributor

ceedubs commented Jan 6, 2017

I'm pretty late to the show, but 👍 from me. I think that this makes it consistent with data types like Validated. It sounds like we have 👍 from @johnynek, but since variance is a touchy subject I'd maybe like to get an opinion from at least one other person. @typelevel/cats what do you think?

@ceedubs
Copy link
Contributor

ceedubs commented Jan 6, 2017

Also, if we merge this, I think that we should do the same for NonEmptyVector.

@johnynek
Copy link
Contributor

johnynek commented Jan 6, 2017 via email

@non
Copy link
Contributor

non commented Jan 6, 2017

I'm 👍 on this. I think a position of variance on data types is fine, but not type classes sounds like something that could work and would be consistent.

If there are concrete problems folks can point to with variance on data types then I'm open to revisiting this position.

@johnynek
Copy link
Contributor

johnynek commented Jan 6, 2017 via email

Also change nonemptyvector's variance
@edmundnoble
Copy link
Contributor Author

Rebased, with NonEmptyVector added to the party.

@johnynek
Copy link
Contributor

johnynek commented Jan 6, 2017

👍

1 similar comment
@kailuowang
Copy link
Contributor

👍

@ceedubs
Copy link
Contributor

ceedubs commented Jan 6, 2017

👍 thanks, @edmundnoble!

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.

6 participants