Skip to content
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

Merged
merged 1 commit into from
Mar 24, 2019
Merged

Conversation

lpaulsen93
Copy link
Contributor

Nothing more to say.

@frlan frlan added this to the 1.34.0 milestone Feb 24, 2018
@frlan frlan requested a review from techee March 11, 2018 21:51
Copy link
Member

@techee techee left a 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.

@techee
Copy link
Member

techee commented Apr 17, 2018

Maybe one question - what Geany API does this change require? Is the currently used API version specified in the plugin enough?

@techee
Copy link
Member

techee commented Apr 17, 2018

Also is the previous API deprecated or something? The patch is kind of "why not" but I slightly fail to see "why yes" reasons.

@lpaulsen93
Copy link
Contributor Author

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.

Maybe one question - what Geany API does this change require? Is the currently used API version specified in the plugin enough?

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.

@techee
Copy link
Member

techee commented Apr 17, 2018

According to https://www.geany.org/manual/reference/legacy.html it's deprecated since Geany 1.26, don't know the API version.

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.

@lpaulsen93
Copy link
Contributor Author

I was asking because if the API version used by the plugin was older, you'd have to bump the API too.

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.

@lpaulsen93
Copy link
Contributor Author

@frlan: according to @techee this PR looks fine so it could be merged. Unless I misunderstood something, there are no pending fixes. The travis CI build failed because of the webhelper plugin and that is not related to my changes.

@frlan frlan merged commit 0654842 into geany:master Mar 24, 2019
@lpaulsen93 lpaulsen93 deleted the geanyctags-newapi branch May 13, 2019 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants