Skip to content

Conversation

@iRagno
Copy link
Contributor

@iRagno iRagno commented Mar 28, 2017

Pull Request Prelude

Changes Proposed

Fixes following Sura job change quest issues:
a) Player will not be able to success the test if fail it (die in test).
b) Waiting room will not get stuck anymore after the first attemp of the quest.

For more details, please read issue.

Affected Branches: master

Issues addressed:

Known Issues and TODO List

None


This change is Reviewable

@MishimaHaruna MishimaHaruna added the status:code-review Awaiting code review label Mar 28, 2017
mapwarp "sword_2-1","yuno_fild07",255,178;
donpcevent "#Sura_garajjom::OnDisable";
donpcevent "Drawing Room::OnEnable";
// donpcevent "#Sura_garajjom::OnDisable";
Copy link
Member

Choose a reason for hiding this comment

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

Why comment and not remove these 2 lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to be the less invasive possible with the script, I comment the line instead of removing it because it is intended to be there (I did comparison with official script), but having it allows player to always success the test.

However, if should be removed, I can remove it without any problem :)

Copy link
Contributor

Choose a reason for hiding this comment

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

if those lines would be commented maybe should add a comment on them that explain why, otherwise if someone in the future compared the script with official one its may be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Applied comment to explain that.

Copy link
Member

Choose a reason for hiding this comment

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

@iRagno If the behaviour of it always succeeding is not official then the old lines should be removed rather than commented.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is not official.. we should check whats wrong with it

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is not official.. we should check whats wrong with it

@dastgirp
Copy link
Member

dastgirp commented Jun 13, 2017

@iRagno any update?
According to smokexyz:

 If the behaviour of it always succeeding is not official then the old lines should be removed rather than commented.

@dastgirp dastgirp added component:scripts Affecting the scripts and NPCs codereview:accepted Code review was positive and the pull request can be accepted as is complexity: 1-straightforward Issue can be resolved easily and removed status:code-review Awaiting code review labels Dec 21, 2018
@MishimaHaruna MishimaHaruna merged commit 68126f7 into HerculesWS:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codereview:accepted Code review was positive and the pull request can be accepted as is complexity: 1-straightforward Issue can be resolved easily component:scripts Affecting the scripts and NPCs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants