antfu-collective / local-pkg

Get information on local packages.
MIT License
153 stars 16 forks source link

fix: deprecate `isPackageExists` in favor of `doesPackageExist` #13

Closed innocenzi closed 3 months ago

innocenzi commented 3 months ago

This pull request deprecates isPackageExists in favor of doesPackageExist, which is grammatically correct. I suggest removing isPackageExists in a major version later, but in the meantime, it will simply call doesPackageExist under the hood.

antfu commented 3 months ago

I am not sure if this only would worth the breaking change. I don't see it as much of a problem as the function names are just conventional identifiers but does not necessary have to be a grammarly correct sentence. The is prefix to me like a sign of boolean utilities. If is this when the library just created, I am happy to rename it. But giving the usage is already wide spread, I don't see it worth to force every user to change it.

innocenzi commented 3 months ago

Definitely agree with you, that's why I am suggesting to deprecate it instead of straight renaming it. You could let the deprecated function here for years, even after the next major version, for that matters — the idea is to let people rename it over time, without pressure.

The is prefix being related to boolean is a great point though. Do you want me to rename doesPackageExist to isPackageInstalled instead?

antfu commented 3 months ago

Yeah. But "deprecating" still gives a signal for the users to migrate - which, to me is not even necessary to bother, as the name wasn't too bad. I might interpret that as `Is that true the package exits where I omit the conjunctions. Anyway, I still think we should do breaking/deprecation when there are real benefits. In this particular case I don't honestly mind either ways, but I think it might be better to be strict to avoid slope effect in the future.

innocenzi commented 3 months ago

Arguably, having grammatically correct function names is a good reason to make a deprecation.

Being reticent to fix small, arguably quite insignificant issues like this is how Apple still has this horrendous typo in their API to this day haha.

Anyway, if you don't feel like fixing it, it's fine. I just think we should aim towards correctness, but at the end of the day, this is not that big of a deal. 👍

innocenzi commented 3 months ago

I renamed it to isPackageInstalled. Feel free to close the pull request if you still don't want the deprecation—I won't be mad, no worries 😅

antfu commented 3 months ago

"grammatically correct" is hard to define - as mentioned, function names are not full English sentences, deps on how you see it, grammar sometimes applies, sometimes doesn't. I agree isPackageExists is not the perfect name, but it's also not that bad - it's not typo nor ambiguity - I bet no one would have trouble understanding it by looking at the name.

Sorry for being a bit stubborn 👀