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

Remove special treatment of non-compliant values in lte and equals #147

Closed
wants to merge 1 commit into from

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Apr 21, 2021

This PR competes with and so closes #154

Instead of arbitrarily returning false, these functions now throw when
given invalid (without the proper type class) input.

@Avaq Avaq force-pushed the avaq/lte-equals branch from 3b30646 to bdd5407 Compare April 21, 2021 11:13
@davidchambers davidchambers force-pushed the avaq/constraints branch 3 times, most recently from 53b5571 to b51561e Compare April 21, 2021 17:43
@Avaq Avaq force-pushed the avaq/lte-equals branch from bdd5407 to 78d3b0a Compare April 21, 2021 18:05
@davidchambers
Copy link
Member

We should update the type signatures of equals, lt, lte, gt, and gte. :)

@Avaq
Copy link
Member Author

Avaq commented Apr 21, 2021

In which way?

@Avaq
Copy link
Member Author

Avaq commented Apr 21, 2021

Oh! I see. Yes.

Base automatically changed from avaq/constraints to master April 22, 2021 07:36
@Avaq Avaq force-pushed the avaq/lte-equals branch from 78d3b0a to 894e541 Compare April 22, 2021 07:40
@Avaq
Copy link
Member Author

Avaq commented Apr 22, 2021

I can't figure out the signature. Hindley Milner is not versatile enough to cleanly separate garbage inputs from desired inputs. The best I can come up with is this overloaded function.

equals :: Setoid a => (a, a) -> Boolean
equals :: (a, AnythingBut a) -> Boolean

@Avaq
Copy link
Member Author

Avaq commented Apr 22, 2021

We could say that the function wants (Setoid a, Setoid b) => (a, b) -> Boolean, but that precludes non-setoid values of differing types.

@davidchambers
Copy link
Member

I suggest using equals :: Setoid a => (a, a) -> Boolean. A clear, imprecise type signature has more pedagogical value than an unclear, precise one. We can document in other ways how the function behaves when given values of different types.

@Avaq Avaq mentioned this pull request Apr 22, 2021
@Avaq Avaq force-pushed the avaq/lte-equals branch from 894e541 to 4e272f4 Compare April 22, 2021 11:43
@Avaq
Copy link
Member Author

Avaq commented Apr 22, 2021

@davidchambers Updated.

@Avaq Avaq marked this pull request as ready for review April 22, 2021 11:44
Comment on lines 1181 to +1118
//. Returns `true` if its arguments are of the same type and equal according
//. to the type's [`fantasy-land/equals`][] method; `false` otherwise.
//. to the type's [`fantasy-land/equals`][] method. Returns `false` for
//. inputs of differing types.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this change is necessary, as we already include the clause if its arguments are of the same type.

Copy link
Member Author

@Avaq Avaq Apr 22, 2021

Choose a reason for hiding this comment

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

But false otherwise is too broad: When a user provides two values of the same type that don't have Setoid, the function throws.

Copy link
Member

Choose a reason for hiding this comment

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

We do not typically document the behaviour of a function when applied to an argument outside its domain. The description of S.trim, for example, does not consider the possibility of application to a non-string value.

Copy link
Member Author

Choose a reason for hiding this comment

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

So you'd prefer to remove the false otherwise part altogether, and treat the special treatment of inputs of differing types as an undocumented "feature", or simply garbage?

Copy link
Member Author

@Avaq Avaq Apr 22, 2021

Choose a reason for hiding this comment

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

when applied to an argument outside its domain

Supplying two values of differing types is within the domain of the function. So is supplying two Setoids of the same type. Anything else isn't.

Copy link
Member

Choose a reason for hiding this comment

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

I do not see how Setoid a => (a, a) -> Boolean permits inputs of differing types. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

But the real signature is:

equals :: Setoid a => (a, a) -> Boolean
equals :: (a, AnythingBut a) -> Boolean

Remember..? But you said that is not pedagogical.

Copy link
Member Author

Choose a reason for hiding this comment

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

The function has a special code path to facilitate the latter overload.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes these inputs part of the domain in my book.

Copy link
Member Author

Choose a reason for hiding this comment

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

You yourself said:

We can document in other ways how the function behaves when given values of different types. -- #147 (comment)

That's what I'm trying to do here.

test/index.js Outdated Show resolved Hide resolved
@Avaq Avaq force-pushed the avaq/lte-equals branch from 4e272f4 to 96ffa06 Compare April 22, 2021 12:36
test/index.js Outdated Show resolved Hide resolved
Instead of arbitrarily returning false, these functions now throw when
given invalid (without the proper type class) input.
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.

2 participants