Skip to content

Conversation

@AnnieRuru
Copy link
Contributor

@AnnieRuru AnnieRuru commented Feb 15, 2019

Pull Request Prelude

Issues addressed

http://herc.ws/board/topic/16526-some-upcoming-hercules-features/?do=findComment&comment=90516

Changes Proposed

All script commands should be in small letters

Affected Branches

  • Master

Known Issues and TODO List

I don't want to memorize another special capital letter case
"All" and "all" is already enough for us -> #784
--> standardize into "all" for script commands -> #2380

so if Haru say again this isn't an issue, feel free to close this PR
for standardization, script commands has to be small letters

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@dastgirp
Copy link
Member

Thanks, I feel haru too wanted script commands in small case.

About "all" that should be case insensitive for now and prefer to have it recommendation that it should be small case

@AnnieRuru
Copy link
Contributor Author

on 2nd thought, actually we can just add "all" and "All" in the script.c

 src/map/script.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/map/script.c b/src/map/script.c
index 7578fcdcc..b1e0a5135 100644
--- a/src/map/script.c
+++ b/src/map/script.c
@@ -11031,7 +11031,7 @@ static BUILDIN(killmonster)
 	int16 m,allflag=0;
 	mapname=script_getstr(st,2);
 	event=script_getstr(st,3);
-	if(strcmp(event,"All")==0)
+	if (strcmp(event, "All") == 0 || strcmp(event, "all") == 0)
 		allflag = 1;
 	else
 		script->check_event(st, event);
@@ -13991,7 +13991,7 @@ static BUILDIN(mobcount)
 	mapname=script_getstr(st,2);
 	event=script_getstr(st,3);
 
-	if( strcmp(event, "all") == 0 )
+	if (strcmp(event, "all") == 0 || strcmp(event, "All") == 0)
 		event = NULL;
 	else
 		script->check_event(st, event);

although after so many years we have already adapted to this ... but its better be late than never

@dastgirp
Copy link
Member

Or change to strcmpi

@4144
Copy link
Contributor

4144 commented Feb 15, 2019

strcmpi will allow all, All, aLL and other combinations...


BUILDIN_DEF(itempreview, "i"),
BUILDIN_DEF(enchantitem, "iii"),
BUILDIN_DEF(expandInventoryAck, "i?"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably wrong names better add with BUILDIN_DEF_DEPRECATED?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh ???
you are the one that made the commit ...
https://github.com/HerculesWS/Hercules/blob/master/npc/other/inventory_expansion.txt
and it used in this script, why deprecate it ?

Copy link
Member

Choose a reason for hiding this comment

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

@AnnieRuru , 4144 probably meant that deprecate commands with wrong names (camel case words) and add new lines for correct name.

Copy link
Contributor Author

@AnnieRuru AnnieRuru Feb 15, 2019

Choose a reason for hiding this comment

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

erm ... this script command was just implemented recently ... right ?
its not like this script command already wide-spread ...
4194f0a#diff-f656dd88296837f04ab24f44eadffa86
hmm ... 2 months old ...
hmmmmmm ........ OK ........

Copy link
Contributor

Choose a reason for hiding this comment

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

near 2 months i think. merged to hercules 1 month ago

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm ... some in-game testing

-	script	khfsdkhjsf	FAKE_NPC,{
OnPCLoginEvent:
	debugmes getInventorySize +"";
}

already throw error like

[Error]: script_add_str: detected possible use of wrong case in a script. Found
'getInventorySize', probably meant to be 'getinventorysize' (in 'npc/zzz.txt').

you sure still need to fix this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1 month++ bump

I have tested this PR will throw error on parse time, with the error shown in above post
@4144

Copy link
Contributor

Choose a reason for hiding this comment

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

please decrecated all commands with wrong case.
for example: BUILDIN_DEF2_DEPRECATED(expandinventoryack, "expandInventoryAck", "i?")

Copy link
Contributor Author

@AnnieRuru AnnieRuru Apr 10, 2019

Choose a reason for hiding this comment

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

nonono, please test this PR, this doesn't need to deprecate
there is already the wrong case detection in the script engine to throw that error already

also, when I tested the BUILDIN_DEF2_DEPRECATED like you said, I did it before
the small letter case got deprecated as well for some reason ...

try it yourself

@Emistry Emistry added component:scripts Affecting the scripts and NPCs component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder labels Feb 26, 2019
@MishimaHaruna MishimaHaruna added this to the Release v2019.06.02 milestone Jun 1, 2019
@MishimaHaruna MishimaHaruna force-pushed the 57-getInventorySize branch from 9120ed9 to b43d122 Compare June 2, 2019 18:51
@MishimaHaruna MishimaHaruna merged commit 20b1d37 into HerculesWS:master Jun 2, 2019
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 component:scripts Affecting the scripts and NPCs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants