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

*delwall now give a friendly warning to remind non-existent wall #2017

Merged
merged 1 commit into from Apr 25, 2018

Conversation

AnnieRuru
Copy link
Contributor

Pull Request Prelude

  • I have followed [proper Hercules code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR will be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

*delwall now give a friendly warning to remind non-existent wall

Affected Branches:

master

Issues addressed:

rathena/rathena#3025
in my battleground script, after I *setwall, but I delwall twice,
and the server didn't even print any error message

Known Issues and TODO List

check the 3rd commit, I missed that delwall thing
and I blame it on the script command itself XD

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@AnnieRuru
Copy link
Contributor Author

AnnieRuru commented Apr 10, 2018

ehh .... woot ... why go to stable branch ... let me recheck what happening ...

EDIT: okies ~

@AnnieRuru AnnieRuru changed the base branch from stable to master April 10, 2018 22:43
@AnnieRuru
Copy link
Contributor Author

rathena/rathena@6ceb1bd
pffff .... I forgot to add the quotation mark ...


if (!map->iwall_remove(name)) {
ShowWarning("buildin_delwall: Non-existent '%s' provided.\n", name);
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

since you already showing warning, isn't it better to return true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rathena there also SCRIPT_CMD_FAILURE, so what's wrong ?
return false just means print an info message of the source npc
#868

Copy link
Contributor

Choose a reason for hiding this comment

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

cos spammy on console kinda

Copy link
Member

Choose a reason for hiding this comment

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

I believe that printing the message about the source NPC makes sense (so you can easily find and fix the broken script that causes the warnings)

@Asheraf Asheraf added this to the Release v2018.05.06 milestone Apr 16, 2018
@Asheraf Asheraf added type:enhancement Issue describes an enhancement or feature that should be implemented component:core:scriptengine Affecting the script engine or the script commands labels Apr 16, 2018
@MishimaHaruna MishimaHaruna merged commit d8c6912 into HerculesWS:master Apr 25, 2018
@AnnieRuru AnnieRuru mentioned this pull request Jan 13, 2019
3 tasks
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 type:enhancement Issue describes an enhancement or feature that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants