Stas Boukarev stassats@gmail.com writes:
It's not apparent when reading the code that it may be redefined somewhere else at some other time.
I don't really think it should be (when reading slime.el), i don't see why the normal slime-inspect function has to have an extra layer of indirection that only the repl code would ever need to use (if there were other places in the code using this variable i would have a different opinion, but there aren't, and i can't see any that would make use of this).
It should of course be apparent when reading the slime-repl.el, but then it is, and the use of defadvice makes it stand out more as a "warning! monkey patching here!" (which it is) rather than just a "oh nothing major, just tweaking a hook" (which it is not).
I see it as an exception to the normal code flow that only applies in certain cases when the slime-repl contrib is loaded, defadvice is the most natural way to express that.
Slime uses variables for cases like this, see `slime-find-buffer-package-function', for example.
yeah, i saw that, and i'm aware of that idiom in slime's code (and in elisp in general). for something like slime-find-buffer-package i can totally see various modes/contribs/languages having different concepts of what a package is and how to find it and wanting to customize that behaviour, that seems like a good hook to have (i have the same opinion about slime-complete-symbol-function and slime-popup-buffer-quit-function), i do not feel the same way about changing slime-sexp-at-point (a sexp is a very well defined thing, you shouldn't be messing with it unless something really weird, like a repl prompt, is going on).
anyway, that's already way too many words, i don't want to have a long drawn out back and forth about it; i will introduce a slime-sexp-at-point-function variable and i'll rewrite the patch to use that.