-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
geanyctags: use new plugin API #708
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, completely forgot about this one. LGTM.
Maybe one question - what Geany API does this change require? Is the currently used API version specified in the plugin enough? |
Also is the previous API deprecated or something? The patch is kind of "why not" but I slightly fail to see "why yes" reasons. |
Please also see the discussion in #703. It's purely for consistency/cleanup. I wonder if there is a new API why not move to it. Otherwise the old one will/can never be deprecated in the future. Anyway - as written in the discussion I would of course accept if you do not like to merge it.
According to https://www.geany.org/manual/reference/legacy.html it's deprecated since Geany 1.26, don't know the API version. But it is NOT obsolete and there is NO plan to remove it. So it will just work fine. So as writen above I will accept a "no" without any discussion and the change is only because I think it's nicer to not use a deprecated API even if support for it will be ongoing. |
I was asking because if the API version used by the plugin was older, you'd have to bump the API too. But I've just checked and 226 which the plugin uses corresponds to the 1.26 release so it's OK for this plugin. But you should check other plugins too. As I said, the patch looks good to me and I have no problem with it - I was just wondering about the reason and you gave me the answer. |
True, thanks for the reminder. The PRs are a bit older, I don't remember if I checked that or not. I will do a re-check some time later. |
Nothing more to say.