joddie/pcre2el

Compilation warning about `reb-target-binding`

Closed this issue · 8 comments

Hi,

Despite the fix in #51, a compilation warning remains, e.g.

In end of data:
magit-todos.el: Warning: the function `reb-target-binding' is not known to be defined.

Because of this code:

https://github.com/mattbeshara/pcre2el/blob/34b35c0ba0f4d0658705591467617e9d7b3c516c/pcre2el.el#L3127-L3129

The byte-compiler doesn't know that the function won't be called on later Emacs versions.

Unfortunately, this still causes some users to report bugs on downstream packages about this warning. It would be good if we could fix it. :)

I've proposed a fix in #56.

There's also the issue that this still used "old advice", resulting in

lib/pcre2el/pcre2el.el:839:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:856:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:870:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:880:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:915:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:923:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:932:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:942:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:964:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:972:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:981:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:3072:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:3081:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:3113:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:3123:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:3137:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’
lib/pcre2el/pcre2el.el:3144:2: Warning: ‘defadvice’ is an obsolete macro (as of 30.1); use ‘advice-add’ or ‘define-advice’

I considered converting this to new advice myself but decided against it because (1) many of these advises perform argument mangling (which increases the risk of mistakes, when converting) and (2) I don't use the (your) packages, which brought in this dependency, often.

Also I am already (kinda) maintaining macrostep by the same author.

Would you be willing to maintain this package, either under your own account or the emacs orphanage. We would have to fork either way. I would take care of updating Melpa and the Emacsmirror, but don't want to otherwise get involved.

Actually, would you even be willing to help out with the orphanage?

Would you be willing to maintain this package, either under your own account or the emacs orphanage. We would have to fork either way. I would take care of updating Melpa and the Emacsmirror, but don't want to otherwise get involved.

Are you asking me, Jonas? If so, I'd say that we should give @hardaker a little time; his GitHub profile shows he's active, and he took care of #51, and this isn't an urgent issue.

Actually, would you even be willing to help out with the orphanage?

I don't know, my plate is pretty full right now. What did you have in mind?

@alphapapa Yes, I was asking you, and you are right, this suggestion was premature. I was a bit on autopilot.

With regards to the orphanage I didn't have anything in particular in mind. Just doing the same things I am doing, so I would have to do less of it. (This [mine above] premature suggestion is just another reminder I am doing too much, and don't have my head in the game enough anymore.)

@alphapapa Yes, I was asking you, and you are right, this suggestion was premature. I was a bit on autopilot.

:)

With regards to the orphanage I didn't have anything in particular in mind. Just doing the same things I am doing, so I would have to do less of it. (This [mine above] premature suggestion is just another reminder I am doing too much, and don't have my head in the game enough anymore.)

I may be willing to help some, depending on the details. Feel free to contact me by email/Matrix/etc. if you'd like. :)

Hi all, a few things:

  1. Sorry for the delay -- I frequently lose track of way too many github notifications in too many repos, so thanks for directly mentioning me. I've tried to notice pull requests but failed this time.

  2. I'd love to have someone take over management of this package generally. I'm not an elisp expert (medium usage myself at best) and I've really just been stepping in to help merge pull requests as @joddie has mostly gone silent.

  3. I can't, however, change the settings for this repo as I don't have admin control over the repo, just merge/commit level permissions.

So the right thing to do might be to fork it to someone that can maintain it, ideally with multiple people in control over the repo to prevent things like this from happening again. We can then put a note in the README of this repo pointing to the new one, and then when you pull the change into the new one you can revert it and keep development there. Reasonable?

@hardaker No need to apologize for the delay. As far as I'm concerned, there wasn't one. This is a minor issue and you merged the PR that fixed it. There's no problem here.

I have more than enough Emacs packages to maintain, and work to do outside of Emacs, already, so I don't want to take on maintenance of another package. If someone wanted to give me commit permission just so I could merge an obviously correct PR once in a while, sure, I'd do that. But I would respectfully decline any responsibility for keeping up with the package's issues and PRs. IOW, I'd be willing to pitch in when it's convenient, but I can't promise anything. I just don't have time for that, and my to-do list is too long already.

Thanks for your work.

Ok, well then I'll keep doing it and will try to prioritize emails about this repo so I see them faster too. I can't change the permissions, but I'll try to reach out to joddie and see if I can get more opening of the repo permissions.

In the mean time, this particular issue (and side issue) I'll mark as closed.

@hardaker sounds good to me!