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

Add optional parameter for *gettimestr #2388

Merged
merged 3 commits into from Apr 7, 2019

Conversation

AnnieRuru
Copy link
Contributor

Pull Request Prelude

Issues addressed

we have *getcalendartime script command, but why our *gettimestr isn't update for it?

Changes Proposed

Add optional parameter for *gettimestr

Tested with

prontera,155,185,5	script	sdkfjhsdkf	1_F_MARIA,{
	mes(gettimestr("%Y-%m/%d %H:%M:%S", 21));
	mes(gettimestr("%Y-%m/%d %H:%M:%S", 21, getcalendartime(0,0)));
	close;
}

which is stated in the documentation

Affected Branches

  • Master

Known Issues and TODO List

none
in fact this is mostly a copy-paste from rathena repo

@HerculesWSAPI
Copy link
Contributor

This change is Reviewable

@AnnieRuru
Copy link
Contributor Author

curious, isn't that after using Malloc should always use Free ?
in this script command, has aMalloc but doesn't have aFree ... isn't that will cause memory leak ?

@4144
Copy link
Contributor

4144 commented Feb 26, 2019

if string next pushed to script engine as string and not cont, script engine will free it if need remove from memory

@AnnieRuru AnnieRuru 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 Feb 28, 2019
@Emistry Emistry added the component:documentation Affecting the documentation in the doc/ folder label Feb 28, 2019
@dastgirp dastgirp added this to the Release v2019.03.10 milestone Mar 9, 2019
@AnnieRuru
Copy link
Contributor Author

hmm ... for some reason this isn't merge ...

@Emistry
Copy link
Member

Emistry commented Mar 11, 2019

btw, cant we make the script command auto calculate the max length parameter? by default its just the same length of the format parameter. Aren't this max length parameter currently look redundant and not necessary?

@AnnieRuru
Copy link
Contributor Author

AnnieRuru commented Mar 11, 2019

hmm ... the person who made this script command probably has die hard habit from C language
char message[CHAT_SIZE_MAX];

then how do you want to modify it ? this is actually a copy paste from rathena, literally
https://github.com/rathena/rathena/blob/78eed02273414a4332f9c088ad7e795a2903ea5f/doc/script_commands.txt#L3155

if we make the <max length> optional parameter, that would become ...
*gettimestr(<format string>{, <max length>{, <timestamp>})
means still have to input the max length ...
or ...
*gettimestr(<format string>{, <max length>})
*gettimestr(<format string>{, <timestamp>})
both fields are INT type, its not like integer/string that we could switch it easily

changing this behavior silently is big no-no ...
try do a search in npc folder ... some scripts already using it

@Emistry
Copy link
Member

Emistry commented Mar 12, 2019

some script already using it...

that doesn't meant we aren't allow to change it or update it? Both rA/Herc and any other emulators has done it countless time. So its not a restriction.

what I meant in previous post are totally remove the max length field.
End Result: *gettimestr(<format string>{, <timestamp>})

For example: changing the code to auto determine the length ?

fmtstr=script_getstr(st,2);
maxlen=script_getnum(st,3);

into

fmtstr=script_getstr(st,2);
maxlen=(int)strlen(fmtstr);

if worry about the length aren't enough for that specific format, then just use the max length allowed.

changing this behavior silently is big no-no ...
try do a search in npc folder ... some scripts already using it

So far only 2 scripts are using it. That's why changelog/commit history exists for a reason. For user to read/keep track of what has been changed. Every milestone release come with a changelog for each release.

@AnnieRuru
Copy link
Contributor Author

I'm not doing that, because your format won't throw error on old scripts
old script that still uses gettimestr("%H%M%S",7); this will print 080007 on your format

@AnnieRuru
Copy link
Contributor Author

just noticed the documentation says strfmtime
when I ask google, says -> Do you mean strftime ?
LOL

@MishimaHaruna MishimaHaruna merged commit 73dacb7 into HerculesWS:master Apr 7, 2019
@AnnieRuru AnnieRuru deleted the 61-gettimestr branch April 10, 2019 13:22
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 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