p5pclub / ref-util

Ref::Util - Utility functions for checking references
6 stars 11 forks source link

API changes to be made #19

Closed wchristian closed 6 years ago

wchristian commented 8 years ago

As discussed on IRC, the API currently does:

Of these, the most commonly useful and intended would be the form taken by is_plain_arrayref, while is_arrayref is likely to cause surprising accidents.

Altogether, the new arrangement will be:

larryl commented 7 years ago

Based on your argument that the current behavior of is_arrayref might be surprising to someone, why not just get rid of it entirely, since is_any_arrayref makes the intention very clear? Seems better to deprecate it entirely than to change the behavior of it and potentially break existing code that uses it now.

FWIW, I'm not sure that argument is valid anyway. The first time I saw this module, I initially assumed that is_arrayref was more encompassing and meant "is_plain_arrayref OR is_blessed_arrayref", since if I cared specifically about blessed vs plain I would use one of the more specific ones, and if I didn't care I'd use the more generic one.

xsawyerx commented 7 years ago

We could optionally remove is_arrayref for a clearer is_any_arrayref, which is what it means. During the last conversation we've had, I suggested to @arc we keep it and don't make this change eventually.

It is not a simple decision to make here. Thank you for adding your input! (It sounds sarcastic, but I'm sincere.)

aaronpriven commented 7 years ago

It seems to me that if there is any possibility that is_arrayref and the like will ever change meaning, it would be best to add is_any_arrayref, etc., ASAP so people can start writing forward-compatible code. If the decision is then ultimately made not to change the meaning of is_arrayref etc., there's not much of a cost in retaining the isany* names (and arguably some benefit for some people in being more explicit).

Thanks for building Ref::Util!

xsawyerx commented 6 years ago

I think the boat has sailed on this and the current situation reflects what I eventually (after enough back and forths) think is correct: is_arrayref should check for an arrayref, blessed or not.

While it more accurate to check if something is a simple arrayref, checking if it's just an arrayref does give more room for the user, allowing them to bless the arrayref if they need to. (I would totally have used it when adding async support to Dancer2 but PSGI didn't support blessed arrayrefs, which it does now.)

Still, at the end, it is the job of the developer to know the options and pick the most accurate test they wish.

I am thus closing this ticket. (And unfortunately, I'm probably still open to this changing, though I think at this point it shouldn't.)