Make NonEmptyVector a value class. #1204

[mikejcurry]

As suggested might be useful in #1204 -
Merging this would make NonEmptyVector a value class, which may be useful in some scenarios. It also:

• Renames variant of concat taking a Vector to concatVector.
• Renames variant of ++ taking a Vector to +++
• Updates test in line with the change.
• Adds additional consistency test.

Updates scaladoc in some minor places, including turning references to concat and concatVector from ++ and +++ respectively into links to the relevant methods.

Not sure about naming, especially +++, but figured a PR would be a good place to discuss:
(a) The naming,
(b) If this is something we would want to do.

[codecov-io]

Current coverage is 89.53% (diff: 100%)

Merging #1212 into master will increase coverage by 0.34%

@@             master      #1212   diff @@
==========================================
  Files           234        234          
  Lines          3144       3143     -1   
  Methods        3085       3091     +6   
  Messages          0          0          
  Branches         57         49     -8   
==========================================
+ Hits           2804       2814    +10   
+ Misses          340        329    -11   
  Partials          0          0          

Powered by Codecov. Last update 885acf6...a5dbc18

[mikejcurry]

Hmm, just noticed #1214, which affects this a lot. I wonder is there a nice way to still be a value class and prevent the issue on #1214 at the same time. Maybe turning NonEmptyVector into a sealed universal trait with a private implementation in the same file. Not sure if this would work though - will have to play around with it. I guess this implementation as it stands is not something that would be good though.

[ceedubs]

@mikejcurry as far as supporting both this and #1214, I think it shouldn't be a problem as long as we make it no longer a case class (as that issue suggests). I think it would look like final class NonEmptyVector[A] private (val toVector: Vector[A]) extends AnyVal. I've left a few comments here about changes we would need to make to support it no longer being a case class.

Unfortunately, there are still some compiler errors related to value classes. Having said that, this seems like a reasonable change to me that I don't really expect to cause trouble. I'm curious whether @non, @johnynek, or @mpilquist have any thoughts.

I feel slightly inclined to use concat and ++ for the Vector varieties and concatNEV/+++ for the NonEmptyVector cases, just because it seems like you'll be more likely to have a Vector than a NonEmptyVector (and the latter is easy to turn into the former), but this is quite arbitrary and I have no strong opinions here.

[mikejcurry]

Hmmm, so I thought (on scala 2.10), that a value class needed exactly one public val parameter in primary constructor and scala 2.11 private was fine. Which is the bit that concerned me. Will do some research/testing as it could have changed/I could be wrong.

[mikejcurry]

Ok, cool, looks like I misinterpreted the implications of the value class limitation with regard to vals, and seems like private constructors work fine - did a quick test and seems like it should fit together.

I can fold the work for #1214 into this also as this would be a bit deficient without it I think. Might be a few days before I get to it though.

[johnynek]

I don't like the +++ syntax, myself. I'm also not 100% that we really need a value class here since changing Vectors involves a lot of allocations anyway. But if we can reduce, that's great.

I agree that if you can only have one arg type, I would take Vector over NonEmptyVector. In fact, I can imagine an implicit conversion from NonEmptyVector to Vector (though it may be too magical). In any case calling a.concat(b.toVector) seems fine.

[ceedubs]

@johnynek I'm not super happy about the +++ syntax either. Maybe we could maybe concatNonEmpty or concatNEV (hmm don't like these names that much either) and just let people use |+| if they want a symbolic operator?

[mikejcurry]

@ceedubs @johnynek @peterneyens

So, based on feedback have done the following:

• Folded in NonEmptyVector unsafe apply method #1214 - NonEmptyVector is now a value class, but not a case class.
• Added test that new NonEmptyVector cannot be called externally, which unfortunately, due to a bug in scala 2.10, can be bypassed on that version. Added related comments in tests and documentation comments.
• No implementation of hashCode or equals required, value classes implicitly delegate to the underlying implementation, i.e. in this case, Vector.
• Removed the +++ operator which felt strange. concat and ++ now take a Vector and concatNEV included as a convenience.

I also bumped the version of scalatest by one to M8. Testing that something doesn't compile was falling foul of some bug in scalatest in M7 which was fixed in M8. Bumping the version to M8 allowed that test to be written. I considered bumping further, but that is more involved, as newer versions of scalatest have more invasive changes that would require bumping versions of discipline etc also - and I didn't want to pollute this PR.

Let me know what you think.

[mikejcurry]

Hmmm, didn't build scala js locally, and I didn't think accessing the version would require a callout like that. Not sure whether to remove the test or factor it into a scala 2.11 specific location. Open to ideas. The test is somewhat useful, but the scala 2.10 defect is a bit of a pain.

[mikejcurry]

Forgot about the catalysts project, so used the Platform.isJvm construct provided with that to only run the test if on JVM, which prevents the test from running under scala js.

The build failure looks like it might be spurious. 137, so the build script was sent a SIGKILL. Perhaps something I've done, but a rebuild might be the best starting point to see if it was just some spurious issue, especially as 2.10 build completed fine.

[ceedubs]

Thanks @mikejcurry! I just restarted the build. Hopefully it works this time.

[ceedubs]

👍 excellent. Thanks again!

[kailuowang]

test description a bit misleading, can remove the word "empty"
"Cannot create a new NonEmptyVector from apply with a vector".

[mikejcurry]

Fair point on the wording. Will rename when I get to a laptop I can work on it from. Hopefully this evening.

[ceedubs]

This is the perfect use-case for living on the wild side and using the in-browser GitHub editor :D

[mikejcurry]

Yea, that's true - :-). I didn't get a chance to get back to this unfortunately. I'll get to it, and the other comments, by the end of the week all going well.

[johnynek]

👍

[adelbertc]

👍

[mikejcurry]

Thanks for merging - I'll follow up on @kailuowang comment - #1212 (comment) separately pretty soon. I just hadn't gotten to it prior to this merge.