Skip to content

Conversation

@AnnieRuru
Copy link
Contributor

@AnnieRuru AnnieRuru commented Feb 19, 2019

Pull Request Prelude

Issues addressed

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

nobody was bother with the "All" and "all" because eathena/rathena is not case-sensitive script engine
during hercules early days, script engine became case-sensitive, and Emistry has asked to change it
Haru said this isn't an issue -> because it mentioned in script-commands
and we are being force to memorize *killmonster -> "All", *mobcount -> "all"

prontera,155,185,5	script	kjhdsfks	1_F_MARIA,{
	monster "this",-1,-1, "--ja--", PORING, 1;
	sleep 1000;
	if ( mobcount( strnpcinfo(NPC_MAP), "all" ) )
		killmonster strnpcinfo(NPC_MAP), "All";
	end;
}

Changes Proposed

Standardize to "all" in script commands

Affected Branches

  • Master

Known Issues and TODO List

hard to tell small or big is better
"this", "--en--", "--ja--", ...
"Leader", "SavePoint", "SavePointAll", "Leader" ...

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@Emistry
Copy link
Member

Emistry commented Feb 26, 2019

imo, keywords shall be non-case-sensitive, or forced all of them into uppercase or lowercase.
Event labels are enforced with case-sensitivity to enforce a better scripting practices for everyone.
However the word "All" are treated as event label since it was used in the event label parameter for the said script command.

Anyway, @AnnieRuru your PR shall fix this line
https://github.com/HerculesWS/Hercules/blob/stable/npc/custom/events/mushroom_event.txt#L44
and any others if exists.

Perhaps the documentations shall mention that keywords "all/All" are usable, but for event label, they still had to follow the On<Event> naming ...?

Last but not least, consider to use strcmpi(event, "all") ? so that it compare the string without need to worry the case-sensitivity for this keyword, and "aLL, AlL, aLl, aLL" are all acceptable for this case? altho its not really necessary. and it look better than having multiple strcmp(...) to compare the strings?

@Emistry Emistry added component:core:scriptengine Affecting the script engine or the script commands component:documentation Affecting the documentation in the doc/ folder labels Feb 26, 2019
@AnnieRuru
Copy link
Contributor Author

  1. dastgir said standardize to have "all" in scripts ....

  2. 4144 don't want to have aLL in scripts ...
    standardize script commands to small letters #2374 (comment)

@MishimaHaruna
Copy link
Member

I agree with Dastgir that this could be standardized this into a lowercase "all".

For backward compatibility it could still accept "All", and print a deprecation warning, with something like:

if (strcmpi(event, "all") == 0) {
	if (strcmp(event, "all") != 0) { // Wrong case, deprecated
		ShowWarning("...");
	}
	allflag = 1;
} else {
	script->check_event(st, event);
}

But that's up to you, or can be done at a later time, I'm fine with it either way for now.

@AnnieRuru
Copy link
Contributor Author

really hard to tell to standardize "all" or "All" in the source code too

Search ""all"" (9 hits in 6 files)
  D:\Ragnarok\Hercules\src\common\socket.c (1 hit)
	Line 1287: 	if( strcmp(str,"all") == 0 ) {
  D:\Ragnarok\Hercules\src\map\battleground.c (1 hit)
	Line 329: 		if( strcmpi(parse,"all") == 0 )
  D:\Ragnarok\Hercules\src\map\map.c (4 hits)
	Line 3507: 	if (strcmpi(mapname, "all") == 0) {
	Line 4295: 			if (strcmp(scriptname, "all") == 0) {
	Line 4906: 			else if (!strcmpi(drop_arg2,"all"))
	Line 5584: 		else if( strcmpi(parse,"all") == 0 ) {
  D:\Ragnarok\Hercules\src\map\npc.c (1 hit)
	Line 4454: 			else if (!strcmpi(drop_arg2,"all"))
  D:\Ragnarok\Hercules\src\map\script.c (1 hit)
	Line 14033: 	if( strcmp(event, "all") == 0 )
  D:\Ragnarok\Hercules\src\map\skill.c (1 hit)
	Line 11981: 		case WZ_QUAGMIRE: //The target changes to "all" if used in a gvg map. [Skotlex]
Search ""All"" (9 hits in 5 files)
  D:\Ragnarok\Hercules\src\map\itemdb.c (2 hits)
	Line 1905: 	if (libconfig->setting_lookup_bool_real(t, "All", &enable_all) && enable_all) {
	Line 1912: 		if (strcmp(job_name, "All") == 0)
  D:\Ragnarok\Hercules\src\map\mapdefines.h (1 hit)
	Line 94: #define MAP_ZONE_ALL_NAME "All"
  D:\Ragnarok\Hercules\src\map\script.c (1 hit)
	Line 11046: 	if(strcmp(event,"All")==0)
  D:\Ragnarok\Hercules\src\map\skill.c (3 hits)
	Line 20635: 	} else if (strcmpi(type, "All") == 0) {
	Line 20736: 	} else if (strcmpi(type, "All") == 0) {
	Line 20874: 		else if(!strcmpi(type,"All")) sk->unit_target = BCT_ALL;
  D:\Ragnarok\Hercules\src\plugins\db2sql.c (2 hits)
	Line 141: 	if (libconfig->setting_lookup_bool_real(t, "All", &enable_all) && enable_all) {
	Line 148: 		if (strcmp(job_name, "All") == 0)

@AnnieRuru AnnieRuru changed the title Allow "all" or "All" for killmonster or mobcount script command Standardize to "all" in script commands Apr 10, 2019
@MishimaHaruna MishimaHaruna merged commit 0c15e81 into HerculesWS:master Jun 1, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants