fp-ts icon indicating copy to clipboard operation
fp-ts copied to clipboard

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

Open fmpanelli opened this issue 4 years ago • 2 comments
trafficstars

🐛 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:

  • keys
  • toArray

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 keys and 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

fmpanelli avatar Sep 08 '21 15:09 fmpanelli

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

gcanti avatar Sep 13 '21 07:09 gcanti

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 keysOrd and 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.

fmpanelli avatar Sep 15 '21 21:09 fmpanelli