joddie / pcre2el

convert between PCRE, Emacs and rx regexp syntax
GNU General Public License v3.0
242 stars 25 forks source link

Use built-in `cl-lib` instead of `a` #47

Closed skangas closed 1 year ago

skangas commented 1 year ago

In 3e235acf95a13124e7c1529e65b520120f13ad9f and 04175c0f85146340b4c56d60b771968c8edabf10, the call to the merge function (from the old built-in package cl) was replaced with a call to a-merge from a. This introduces a dependency on a package that is not on GNU ELPA or NonGNU ELPA, meaning that users can't install pcre2el from there without adding another package archive (usually MELPA).

Luckily, we can just use the built-in cl-merge instead, which is a drop-in replacement for the old merge function. That function is in the built-in package cl-lib, which is the replacement for the old cl package. Note that the move from cl to cl-lib is basically just a rename - the behaviour should be exactly the same as before. (If it's not, that's likely to be a bug.)

So I make that change here. Note that since we depend on Emacs 25.1, there is no need to depend explicitly on cl-lib (as it comes built-in with that version), so I remove that redundant dependency also. I also bump the pcre2el version so that a new release is made on NonGNU ELPA, with the fixed dependency.

Please consider merging this. Thanks in advance.

See also: https://github.com/plexus/a.el#should-you-use-it-for-new-code

skangas commented 1 year ago

@hardaker When you have a chance, perhaps you might want to take a look at this. Thanks.

hardaker commented 1 year ago

thanks for the fix. merging.

piyo commented 1 year ago

There is a problem with this recent set of changes.

The (require 'a) still needs to be removed.

Separately, the original code uses cl-labels so it was not referring to a global function merge. So it was not referencing the global cl.el merge function. Replacing the text with a-merge or cl-merge does not change the meaning of the code. I suggest replacing it with local-merge or something else that indicates it is using a locally-defined function.

skangas commented 1 year ago

@piyo I think you're right, the require needs to be changed, and we could rename it to be less confusing. Please see #50.

Thanks.