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

- implement Contains Wart for List and Option #566

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AlexanderRay
Copy link

Current situation:

List.contains and Option.contains operations are not type-safe, which means it is possible to compile List(1).contains("1")

this wart disables contains in List's and Options

it was also suggested here - #48

package warts

object Contains extends WartTraverser {
private[warts] def message: String = "Don't use List or Option `contains` method because is not typesafe"
Copy link
Contributor

Choose a reason for hiding this comment

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

actually they are type safe, just not very useful.

Copy link
Author

Choose a reason for hiding this comment

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

it is possible to implement something like List(1).contains(true) without any warning about it

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree a warning would be a good thing - my issue is with the wording, because technically it is type safe: you'll never get a ClassCastException at run time, the result is correct, and there are even legitimate use cases for this behaviour. just not very many.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, can you suggest better wording for the warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, i'm not even sure i'd want to disable these functions without some more useful replacement - which if it existed, should probably be suggested in the message

Copy link
Author

Choose a reason for hiding this comment

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

contains in Lists and Options is type unsafe it is possible to implement List(1).contains("1"). Use type-safe implementation for example from cats List.contains_

something like that?

`contains` in Lists and Options is type unsafe it is possible to implement List(1).contains("1"). Use type-safe implementation for example from cats `List.contains_`
````scala
// Won't compile: List.contains is disabled
List(1).contains(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

but this is the useful case, isn't it? it might make sense to only disable them for un-related types...

Copy link
Author

Choose a reason for hiding this comment

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

that is generally a bad idea to use native contains , my opinion is that always something like cats.contains_ should be used to be safe

@xuwei-k
Copy link
Collaborator

xuwei-k commented Dec 5, 2023

FYI 37e9823

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants