Minebench / Tresor

An experimental VaultAPI compatible abstraction layer for Bukkit/Spigot/Paper plugins
https://ci.minebench.de/job/Tresor/
GNU General Public License v3.0
8 stars 3 forks source link

[#9] Placeholder service provider #30

Open casptyche opened 1 month ago

casptyche commented 1 month ago

Hi,

9 - Apologies I didn't see another PR had been raised for this already & don't intend to create a 'race' of any sorts - I'm not looking for the IssueHunt.

I've implemented a very rough approach and just implemented an abstraction layer directly for PlaceholderApi and maintained PlaceholderApi's approach of leaving the translation of 'params' completely up to the external implementation, rather than looking to create a mapping within Tresor, though I appreciate this may not be what was in mind?

The end-goal in my head is that when adding other Providers in the future, a translation can be created between PAPI's format and their format. Though I'm not aware of other providers / Placeholder plugins, I had a brief look at MVDW but it appeared that it's exclusively for MVDW's premium plugins and are all available via PAPI regardless?

Let me know, I'm quite keen to contribute to some open source repos and this seemed like a good issue to pick up but if I've missed the intended mark I'm happy to adjust this or look at another issue (given the other PR raised for #9 too).

casptyche commented 1 month ago

Marking as draft as code is untested

Phoenix616 commented 1 month ago

Thank you for your PR. Even though it's a duplicate it's quite interesting to see a different approach. You seem to be missing a way for another plugin to resolve/replace placeholders though? (You don't need to add that if you want to do something else now that you noticed there's a PR for it already. Depends on if @eyvallahabi wants to continue their PR I guess)

But I like the way placeholders can be registered by other plugins in comparison to the other PR, I just feel that it might be more complex than necessary as it basically just mirrors what PAPI does. (I don't really see the usefulness of the author, version and plugin being exposed in the API. Imo. that's an implementation detail of the service provider so a simple Placeholders#registerPlaceholder(Plugin, String id, BiFunction<OfflinePlayer, String, String>) might even be able to handle it with the PAPI extension being constructed inside the provider implementation)

@eyvallahabi maybe you can find a similar but simpler approach for your PR?

casptyche commented 1 month ago

Thank you for your PR. Even though it's a duplicate it's quite interesting to see a different approach. You seem to be missing a way for another plugin to resolve/replace placeholders though? (You don't need to add that if you want to do something else now that you noticed there's a PR for it already. Depends on if @eyvallahabi wants to continue their PR I guess)

But I like the way placeholders can be registered by other plugins in comparison to the other PR, I just feel that it might be more complex than necessary as it basically just mirrors what PAPI does. (I don't really see the usefulness of the author, version and plugin being exposed in the API. Imo. that's an implementation detail of the service provider so a simple Placeholders#registerPlaceholder(Plugin, String id, BiFunction<OfflinePlayer, String, String>) might even be able to handle it with the PAPI extension being constructed inside the provider implementation)

@eyvallahabi maybe you can find a similar but simpler approach for your PR?

Thanks for taking the time to reply and look over. You're totally right, I forgot to implement an equivalent of PAPI's setPlaceholders().

My thinking behind copying over the author/version/plugin was that then a server owner or dev or whatever could then trace a placeholder through their provider (PAPI/whomever) to the original, rather than it being traced back to Tresor. But I do agree it can be removed for the sake of simplifying this PR, and I did feel like I was being too verbose essentially copying over PAPI's interface to this MR :D.

The callback approach you mention probably be the most plugin agnostic approach, would be happy to adopt that too. If the other PR user doesn't come back in the next week or so I'll look at picking this back up, don't want to hijack something they're half-way through.