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

feat(platform): add clear methods to remove the audience provider cache #65

Merged
merged 1 commit into from Nov 25, 2021
Merged

Conversation

Silthus
Copy link
Contributor

@Silthus Silthus commented Oct 28, 2021

Added [...]Audiences.clear() and [...]Audiences.clear(Plugin) to all three audiences. This will remove the audience from the static cache. This functionality may be needed by test environments.

This is related to this request on discord.

@zml2008
Copy link
Member

zml2008 commented Oct 29, 2021

this is not an audience cache, this is an AudienceProvider cache........ call things the right name

imo the correct solution is to make the AudienceProvders implement Closeable and have a close() method that removes them from the cache, not what's currently here

@Silthus
Copy link
Contributor Author

Silthus commented Oct 29, 2021

this is not an audience cache, this is an AudienceProvider cache........ call things the right name

imo the correct solution is to make the AudienceProvders implement Closeable and have a close() method that removes them from the cache, not what's currently here

Mmh but shouldn't Closable, better AutoClosable be used for locking resources and releasing them, like IO operations? The AudienceProvider isn't really locking anything, it just holds a reference (cache) to the Audiences created. Also Autoclosable throws an Exception by default, which is not really the case here.

Maybe rename it to remove and keep the clear to match the overall Collection syntax?

@Silthus Silthus changed the title feat(platform): add clear methods to remove the audience cache feat(platform): add clear methods to remove the audience provider cache Oct 29, 2021
@Silthus
Copy link
Contributor Author

Silthus commented Oct 29, 2021

I have built a workaround in my testing library in the mean time:

Field instances = Class.forName("net.kyori.adventure.platform.bukkit.BukkitAudiencesImpl").getDeclaredField("INSTANCES");
instances.setAccessible(true);
((Map<String, BukkitAudiences>) instances.get(null)).clear();

Should this API addition still be added or should I close the PR and the users who need to clear the cache use the reflection code above?

@kezz
Copy link
Member

kezz commented Nov 24, 2021

I might be missing something, but why can't you just clear the instance from the static map when the existing close method is called?

@Silthus
Copy link
Contributor Author

Silthus commented Nov 24, 2021

I might be missing something, but why can't you just clear the instance from the static map when the existing close method is called?

No you are right, I am the one that missed something ;)
I did not see the existing close method.

@Silthus
Copy link
Contributor Author

Silthus commented Nov 24, 2021

Ok I updated the PR so that the AudienceProvider removes itself from the cache when close() is called. Thank you @kezz

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now!

@kezz kezz requested a review from zml2008 November 24, 2021 20:20
@zml2008 zml2008 self-assigned this Nov 25, 2021
@zml2008 zml2008 added this to the 4.0.1 milestone Nov 25, 2021
@zml2008 zml2008 merged commit f7b1731 into KyoriPowered:master Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants