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

SiegeRemoveEvent and punctuation fixes #871

Merged
merged 11 commits into from
Jul 21, 2023

Conversation

ewof
Copy link
Contributor

@ewof ewof commented Jul 20, 2023

Description:

  • add a siege remove event so plugins can handle when sieges are remove by immunity running out or via admin command
  • fixes some weird !. punctuation's in en-US

New Nodes/Commands/ConfigOptions:


  • I have tested this pull request for defects on a server.

By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the SiegeWar License for perpetuity.

ewof added 3 commits July 20, 2023 01:49
…heck in isPlayerInTimedPointZone

SiegeRemoveEvent so plugins can listen when a siege is removed via admin command or after SiegeImmunity runs out
@ewof ewof changed the title Removeevent wildernessbannertoggle SiegeRemoveEvent, Config option for allowing banner control in towns Jul 20, 2023
@ewof ewof changed the title SiegeRemoveEvent, Config option for allowing banner control in towns SiegeRemoveEvent, Config option for allowing banner control in towns, punctuation fixes Jul 20, 2023
@Goosius1
Copy link
Collaborator

Goosius1 commented Jul 20, 2023

🚧
Hi,
As clarified on Discord yesterday:

  • For the siege removal event, that's no problem. I presume that event would also be fired when the Siege is naturally removed after SiegeImmunity runs out.

  • For the config option on moving the flag, no unfortunately we can't add that yet. I don't want to provide such a config in the plugin unless/until a robust and well-tested overall town-invasion system is ready to roll out. The reason is that if that was provided, some server's would use it, many could end up in mess, which would negatively effect the reputation of the team and system. Thus please remove that config from this PR, then I will resume the review.

@Goosius1 Goosius1 self-requested a review July 20, 2023 09:09
@Goosius1 Goosius1 added this to the 2.7.1 milestone Jul 20, 2023
@Goosius1 Goosius1 added the enhancement New feature or request label Jul 20, 2023
@ewof ewof changed the title SiegeRemoveEvent, Config option for allowing banner control in towns, punctuation fixes SiegeRemoveEvent and punctuation fixes Jul 20, 2023
Copy link
Member

@LlmDl LlmDl left a comment

Choose a reason for hiding this comment

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

I think it makes more sense to throw the remove event from within the SiegeController.removeSiege() method, as there's still 3 places that call removeSiege(siege) which are not going to throw the event.

src/main/resources/lang/en-US.yml Outdated Show resolved Hide resolved
src/main/resources/lang/en-US.yml Outdated Show resolved Hide resolved
src/main/resources/lang/en-US.yml Outdated Show resolved Hide resolved
src/main/resources/lang/en-US.yml Outdated Show resolved Hide resolved
src/main/resources/lang/en-US.yml Outdated Show resolved Hide resolved
@ewof ewof requested a review from LlmDl July 20, 2023 14:10
@ewof ewof mentioned this pull request Jul 20, 2023
1 task
@ewof ewof requested a review from LlmDl July 20, 2023 22:11
Copy link
Member

@LlmDl LlmDl left a comment

Choose a reason for hiding this comment

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

Found one last typo in a lang file.

src/main/resources/lang/en-US.yml Outdated Show resolved Hide resolved
Copy link
Member

@LlmDl LlmDl left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll leave this in Goosius' hands to merge.

@Goosius1
Copy link
Collaborator

Thanks so much Llama!. I'll do a quick pass secondary review on this now.

@Goosius1 Goosius1 merged commit b23f22f into TownyAdvanced:master Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants