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

TFM 5.0.2 Release #2176

Open
wants to merge 16 commits into
base: devel
Choose a base branch
from
Open

TFM 5.0.2 Release #2176

wants to merge 16 commits into from

Conversation

Wild1145
Copy link
Member

No description provided.

@Wild1145 Wild1145 added this to the 5.0.2-Alpha 1 (MC 1.13) milestone Mar 17, 2019
@Wild1145 Wild1145 requested a review from JeromSar March 17, 2019 11:49
Copy link
Member

@JeromSar JeromSar left a comment

Choose a reason for hiding this comment

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

All good, apart from a small mistake.

src/main/resources/config.yml Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Sep 14, 2019

CLA assistant check
All committers have signed the CLA.

@Wild1145 Wild1145 requested a review from JeromSar September 14, 2019 18:49
{
// Load banned tags
blockedTags.clear();
blockedTags.addAll((Collection<? extends String>) ConfigEntry.BLOCKED_TAGS.getList());
Copy link
Member

Choose a reason for hiding this comment

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

You don't need a separate service for loading this list. Especially since we're never adding to it. You can simply use ConfigEntry.BLOCKED_TAGS.getList() anywhere you need it. Lookup should be pretty fast and without I/O.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just going to go and facepalm. Apparently I decided over-engineering it was the best way. Thanks for the heads up, I'll make appropriate changes.

@Focusvity
Copy link

@Wild1145 you didn’t modify to dependency to point to EssentialsX.

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.

4 participants