emacs-lisp-style-guide
emacs-lisp-style-guide copied to clipboard
bbatsov
Discourage usage of version defconst in lieu of lm-version
Here's a possible complication: it's hard to use a constant wrong.
I'm kinda worried that someone will end up calling company-version in a loop. But even once per command wouldn't be too good.
@dgutov I've used lm-version in Flycheck since ages, and never had any issues reported to me.
@lunaryorn Me neither.
Oh well, maybe it's fine. I don't like the idea of some code reading company.el each time the user does something, but it takes only 0.002s on my machine anyway.
I'm kinda worried that someone will end up calling company-version in a loop. But even once per command wouldn't be too good.
You can always use lm-version to initialize a version variable.
@lunaryorn About never. It's a hypothetical concern.
@bbatsov Can or should?
@dgutov It'd vote for “can”, as in “if you really, absolutely, for some mad reason, need to read your version number every second”… otherwise just stick to lm-version and don't worry.
@lunaryorn Here's a plausible scenario: suppose in 0.9 I change company-backends interface in an incompatible way. And some backend author decides to make it behave one or the other way depending on the value company-version returns. If they call it each time company-foo is called, this will be a lot of insert-file-contents. Probably won't be too slow anyway, but still.
Maybe the style guide should advise about calling xpkg-version function too often, too.
@bbatsov Can or should?
Depends on how worried one is about the performance. :-)
@dgutov I'd argue that the backend author should check for a feature rather than a version number—i.e. use fboundp instead of lm-version.
@lunaryorn Suppose tomorrow, company-backends elements would have to confirm to the standard "completion table" interface, instead of the current one. What kind of fboundp check could help them to support both versions of company-mode?
@dgutov That'd be a bad change, specifically because it's almost impossible for downstream projects to detect that.
If you need to switch to a different interface, you should much rather introduce a new variable, i.e. company-completion-tables, where backends can register for the alternative interface, or continue to support both interfaces in company-backends, and let backends add a special property to declare that they support the new interface.
@lunaryorn How about a smaller change, where the new version company-mode supports both interfaces, but one action allows a richer/neater/more efficient calling convention, like what adding asynchronous calling support for candidates was? The backend is allowed to do one things in a better way, but how would it know about it?
Suppose we also want to get rid of doing things the old way at some point? Then passing in an extra argument with a special value, etc, is not a very attractive proposition.
@dgutov I know too little about Company to really discuss this matter. At a last resort, you can always export a constant, i.e. company-supports-cool-interface or whatever. That's still better than forcing backends to check the Company version.
But I stick to my opinion that any backwards-incompatible change which forces downstream packages to make such checks is a bad one. You don't avoid “bad” code with such changes, you're just pushing it to downstream packages.
Okay then, here's a different question: if we don't consider programmatic use, why have a constant or a function at all?
The user can just as well M-x describe-package flycheck. Or should be able to. Is is just because of MELPA version mangling?
@dgutov Provided that the user installed Flycheck with package.el, yes. M-x flycheck-version exists purely for convenience.
I agree with @dgutov in that you can get into a situation where you need to frequently know another package's version. But my answer to that would be to query this version at load time and stick that in a constant.
You can use eval-after-load to ensure this constant stays up to date when the user upgrades.
Ok, let's prescribe using lm-version for the version functions, and if we see the performance problems in the wild, then add a section about using the -version functions from other packages.