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

Reconsider overloading in NonEmptyVector #1204

Closed
ceedubs opened this issue Jul 15, 2016 · 2 comments
Closed

Reconsider overloading in NonEmptyVector #1204

ceedubs opened this issue Jul 15, 2016 · 2 comments

Comments

@ceedubs
Copy link
Contributor

ceedubs commented Jul 15, 2016

Currently NonEmptyVector has overloaded concat and ++ methods supporting both NonEmptyVector and Vector as the arguments.

In general I'm a bit skeptical about overloading. In this specific case the main downside I see is that it prevents us from turning NonEmptyVector into a value class (which seems like it might be an otherwise reasonable thing to do).

What do people think?

@mikejcurry
Copy link
Contributor

mikejcurry commented Jul 16, 2016

👍 on removing the overloading as a general principle. Also, having a value class would be useful in specific use cases.

The syntax is still useful for working with Vectors directly though. Maybe a rename to concatVector or appendVector and a different operator such as +++.

As an aside, with regard to value classes, it is worth bearing in mind that, as I understand at least, invoking any general code that involves summoning an instance would negate the benefit of the value class. i.e. summoning the Semigroup for NonEmptyVector would require an instance of NonEmptyVector to be instantiated - because NonEmptyVector would be being used as a type argument to Semigroup and one of the limitations on value classes is that if they are used as type argument an instance needs to be instantiated. I think having NonEmptyVector as a value classes is still useful despite this, because it opens up options to users of the class.

@mikejcurry
Copy link
Contributor

Believe this should be resolved by #1212

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

No branches or pull requests

2 participants