search

ssbc / ssb-keys

keyfile operations for ssb
36 stars 26 forks source link

Move optional arguments after mandatory arguments #67

Open christianbundy opened 4 years ago

christianbundy commented 4 years ago

The signature and verification methods have an optional hmac_key, which is the second argument, but the third argument is mandatory. Passing null everywhere sucks and we should move optional arguments after mandatory arguments.

davegomez commented 4 years ago

@christianbundy this is "kind of" already doing it in the first line of each function.

...
  if(!obj) obj = hmac_key, hmac_key = null
...

If the third parameter is null/undefine sets obj with the value of hmac_key and hmac_key with null.

Obviously this is not safe at all but 🤷‍♂️

christianbundy commented 4 years ago

Yep. I think that's overly complicated and makes the API harder to learn / maintain. In my experience, mandatory arguments always come before optional arguments so that you don't have to do stuff like that.

staltz commented 4 years ago

I fully agree with the change, but it being a breaking change, I'm afraid of cases where dependents just update the version of ssb-keys without updating their code, and then the arguments would be passed, but no error would be emitted, instead the semantics of the arguments are flipped. This may sneakily change the semantics of dependents in a way that's hard to trace.

This package being very important for reliability in the SSB ecosystem, I would actually prefer a stable API (even if quirky) over a simple and nice API.

davegomez commented 4 years ago

@staltz I have an idea on how to work on this keeping backward compatibility. I'll publish a PR this weekend so you guys can take a look and decide if the approach is something we want to keep.