ankurdave / color-identifiers-mode

Emacs minor mode to highlight each source code identifier uniquely based on its name
308 stars 23 forks source link

Rm extra indirection by making (color-identifiers:refontify) an alias #122

Closed Hi-Angel closed 6 days ago

Hi-Angel commented 1 week ago

For Emacs 25.1 and higher that makes (color-identifiers:refontify) an alias to (font-lock-flush). (color-identifiers:refontify) always was just a wrapper, so no functional change is intended besides removing the unnecessary indirection.

I haven't found a way to just leave (if (fboundp … (defalias …) expression at the top-level and not cause any byte-compilation errors, so the solution required a little bit of hackery, but hopefully it should be safe. At least tests are passing and I haven't found any problems.

Hi-Angel commented 1 week ago

@purcell since you're the author of the idea with aliasing font-lock (for hcl-mode):

(if (fboundp 'font-lock-flush)
    (defalias 'color-identifiers:refontify 'font-lock-flush)
  (defun color-identifiers:refontify ()
    (with-no-warnings (font-lock-fontify-buffer))))

I figured I'd CC you in case you have any solution. I tried implementing your idea, but it gives either this:

Error: the function ‘color-identifiers:refontify’ is not known to be defined.

or if I wrap it to (cl-eval-when 'compile that gives:

Error: the function ‘color-identifiers:refontify’ might not be defined at runtime.

So I ended up doing the hackery that can be seen in commit Rm extra indirection by making (color-identifiers:refontify) an alias. I'd like to avoid hacks of course, so Just wondering if you might know a proper solution to that.

Thanks in advance!

Hi-Angel commented 1 week ago

Per my understanding, in hcl-mode that wasn't a problem because actual calls to the alias were in different .el files. So when those files were getting compiled, the one with the alias was always loaded.

But here everything is in a single file, and I presume in this case splitting it just for the sake of making an alias not cause compilation errors would be an even larger hackery 😅

purcell commented 6 days ago

General observation here is that the current code in master is not well-packaged, since it advertises compatibility with Emacs 24, but the code actually requires at least Emacs 24.4. Given that, I'd suggest depending on Emacs 25.1, and then you don't need the defalias for font-lock-flush at all.

Hi-Angel commented 6 days ago

You might be right. Although, 24.4 is just a minor release of 24, but it indeed should be "24.4" then. And either way, 24.4 is 10 years old at this point (soon will be 11), it's very unlikely anybody's still using it. So might as well just drop the code.

OTOH, I am partly interested in the correct solution (disregarding if we drop Emacs 24) because I grepped over the code at myelpa/ dir and found a few more patterns like the one here. And I figured it might be an easy fix to send to other projects as well.

Hi-Angel commented 6 days ago

Well, running package-lint over the file shows that Emacs 24.x releases weren't so minor… Apparently they had lots of features added, and many of them are used in the mode.

So anyway, I added a commit correcting the version, to 24.4 for now. I tried integrating package-lint as well, but wasn't successful because from what I see the mode doesn't allow disabling some of the warnings — at least not the contains a non-standard separator ':' warning — and I definitely won't be able to rename identifiers as it's basically stable API.

Hi-Angel commented 6 days ago

and I definitely won't be able to rename identifiers as it's basically stable API.

That said — not that it matters given I can't rename it — but in my opinion using colons was a neat idea. Given that Emacs doesn't allow "local functions", so every function had to be prefixed with the plugin name to avoid naming clash, the colon allows to clearly separate that "workaround" part of the name from the actual function or variable name.

purcell commented 5 days ago

That said — not that it matters given I can't rename it — but in my opinion using colons was a neat idea.

It's cool to have an opinion or personal preference, and if Emacs were reinvented now perhaps things would be different, but the Emacs coding conventions explicitly establish two hyphens as the preferred separator for "private" symbols, and generally it is preferable to value community consistency over individual aesthetics. That's why package-lint is pedantic about this. I'm not actually sure how this package got into MELPA given its use of colons — we'd normally require package-lint issues to be fixed.

purcell commented 5 days ago

BTW, it is possible to migrate from : to -- and satisfy package-lint — the functions can be renamed, and then a define-obsolete-{function,variable}-alias can be used to keep the old name around.

Hi-Angel commented 5 days ago

BTW, it is possible to migrate from : to -- and satisfy package-lint — the functions can be renamed, and then a define-obsolete-{function,variable}-alias can be used to keep the old name around.

It's a lot of work, and then how long are we gonna keep obsolete names around, another ten years? Besides, obsoletion in plugins usually goes unnoticed. That's because to find out that you're using an obsolete variable/function you'd need to byte-compile a file, but many people don't byte-compile their .emacs config (because then you have to remember to re-compile it on every change. I was doing that when I first started using Emacs, but at some point just stopped.). So people wouldn't even know.

Hi-Angel commented 5 days ago

but the Emacs coding conventions explicitly establish two hyphens as the preferred separator for "private" symbols

Btw, these are conventions for code internal to Emacs, not for Elpa or plugins. I'm just saying this because that implies you can see "in the wild" code that doesn't accord to the conventions.

purcell commented 5 days ago

It's a lot of work, and then how long are we gonna keep obsolete names around, another ten years?

It's been done before in much more popular packages than this, so I can assure you it's perfectly possible.

But I really don't care here. It's not a great use of my time to engage further, sorry, just sharing my experience from reviewing thousands of packages as part of building up MELPA from 2012.

Hi-Angel commented 5 days ago

It's been done before in much more popular packages than this, so I can assure you it's perfectly possible.

There's a difference to changing/obsoleting a single variable/function, or doing that to everything in the entire plugin 😊

But I really don't care here. It's not a great use of my time to engage further, sorry, just sharing my experience from reviewing thousands of packages as part of building up MELPA from 2012.

It's okay, I understand.