gcanti / fp-ts

Functional programming in TypeScript
https://gcanti.github.io/fp-ts/
MIT License
10.9k stars 503 forks source link

Record.ts - some methods don't have Ord as input #1577

Open fmpanelli opened 3 years ago

fmpanelli commented 3 years ago

🐛 Bug report

Current behavior

Where it makes sense, most of the Record.ts methods take an Ord as input which is used to sort the keys. The corresponding older method overloads that used a fixed sorting based on string.Ord are deprecated.

A couple of methods are not aligned with this and instead use string.Ord, in particular:

I am not sure it should be necessarily considered a bug. It just looks a bit asymmetric.

Expected behavior

I'd expect the methods to take an Ord as input parameter in line with other methods (e.g. compare toArray with collect).

Reproducible example

Not applicable.

Suggested solution(s)

I suggest to create two new overloads for keysand toArray taking an Ord as input and to deprecate the current overloads.

Additional context

Not applicable.

Your environment

Which versions of fp-ts are affected by this issue? Did this work in previous versions of fp-ts?

Software Version(s)
fp-ts 2.11.1
TypeScript 4.4.2
gcanti commented 3 years ago

I suggest to create two new overloads for keys and toArray taking an Ord as input and to deprecate the current overloads.

This was considered while adding Ord to the other functions but AFAIK there's no reliable way to do it

fmpanelli commented 3 years ago

Thanks Giulio for your answer.

If I understand correctly, you are stating that there is no reliable way to construct the two overloads because you cannot distinguish if the argument received is a Record or an Ord. In fact Ord is a Record too.

An option that comes to my mind to overcome this obstacle would be to create alternative methods, with alternative names such as keysOrdand toArrayOrd, which always require Ord as the first argument. Not a very satisfactory solution, either.

Another option would be to break back-compatibility and remove the current methods in v3 replacing them with methods that always require an Ord.

That's all I have on this at the moment. Thank you for taking the time to clarify the topic.