Skip to content

Conversation

@Kenpachi2k13
Copy link
Member

@Kenpachi2k13 Kenpachi2k13 commented Jan 5, 2020

Pull Request Prelude

Changes Proposed

  • Added a flag to ignore mapflag restrictions in warpparty() and warpguild() script commands.
  • Applied code style to warpparty() and warpguild() script commands.
  • Updated doc/script_command.txt.

Issues addressed: #1861

@Kenpachi2k13 Kenpachi2k13 added status:code-review Awaiting code review component:core:scriptengine Affecting the script engine or the script commands labels Jan 5, 2020
@Kenpachi2k13 Kenpachi2k13 added this to the Release v2020.01.12 milestone Jan 5, 2020
@Kenpachi2k13 Kenpachi2k13 self-assigned this Jan 5, 2020
Copy link
Contributor

@Asheraf Asheraf left a comment

Choose a reason for hiding this comment

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

So this silently breaks every script that relies on these commands checking the map restrictions, you can either introduce a flag to enable/disable these checks (defaults to enabled), or go with the suggestion in the related issue to deprecate these commands and merge them into warp().

@Kenpachi2k13
Copy link
Member Author

Kenpachi2k13 commented Jan 6, 2020

There is no script that relies on that restrictions. warpguild() isn't used in any script and warpparty()is only used in F_CashPartyCall().
Furthermore; from my point of view, checking things like this is part of the script logic and should not be done inside the called function.
In my opinion, merging all warp commands into one seems to be a very bad idea. It would cause a parameter overload. And it would also cause that the meaning of one parameter depends on the value of another parameter (flag for passed ID is a guild or party ID) which isn't good programming practice.

@Asheraf
Copy link
Contributor

Asheraf commented Jan 7, 2020

I am not talking about the scripts included with the repository (those can always be fixed), but the tens of thousands of scripts written by the community since these commands were introduced a long long time ago, unfortunately opinions on how it should work does not matter here, the scripting engine MUST ALWAYS keep backward compatibility, unless you stop the script from loading you cannot change the behavior in a manner that breaks that compatibility.

@Kenpachi2k13
Copy link
Member Author

Ah, okay. You're right. Thanks for pointing that out.
Requested changes are committed, please re-review.

As a side note:
The documentation for warpparty() implies that <include leader> can be passed without passing <from map name>, which is wrong but makes sense.
I'm indecisive, if I should repair this within this PR or in a new one when this one was merged.
Feel free to give an advice.

@Kenpachi2k13 Kenpachi2k13 changed the title Remove mapflag restrictions from warpparty() and warpguild() script commands. Add flag to ignore mapflag restrictions in warpparty() and warpguild() script commands. Jan 7, 2020
@Kenpachi2k13 Kenpachi2k13 requested a review from Asheraf January 7, 2020 23:49
@Kenpachi2k13 Kenpachi2k13 added the component:documentation Affecting the documentation in the doc/ folder label Jan 8, 2020
@Asheraf
Copy link
Contributor

Asheraf commented Jan 8, 2020

Hmm im not sure if i like inserting the new argument in the middle and that offset changing even though it works, anyways you missed updating the building definition with an extra optional argument.

@Kenpachi2k13
Copy link
Member Author

I did not put the flag at he end of the argument lists, because this would force the user to pass a <from map name> (and for warpparty() also <include leader>), too, which makes no sense.

@Kenpachi2k13 Kenpachi2k13 requested a review from dastgirp January 9, 2020 22:50
@MishimaHaruna MishimaHaruna merged commit 052cbc8 into HerculesWS:master Jan 12, 2020
@Kenpachi2k13 Kenpachi2k13 deleted the issue#1861 branch January 14, 2020 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder status:code-review Awaiting code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants