-
Notifications
You must be signed in to change notification settings - Fork 9
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
More formatting options #21
base: master
Are you sure you want to change the base?
Conversation
Hi, would you like to add a timezone variable to |
Value v = ss.pop(); | ||
Object o = null; | ||
switch (v.type) { | ||
case Null: o = Value.null_(); break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong, should just be o = null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong, should just be
o = null
iirc I used this because I saw Value.null_()
being used somewhere else. Anyways do you want me to edit and fix this, if you are going to accept this pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me personally it would be really nice, if this PR is being merged. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong, should just be
o = null
iirc I used this because I saw
Value.null_()
being used somewhere else. Anyways do you want me to edit and fix this, if you are going to accept this pr?
Value.null_()
is the starscript null type, but in this case you’re feeding it into the java String.format()
method, which takes in the java null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check my new commits in case you're interested in accepting this PR. Should be fixed now.
Java |
Yeah, I mean the option to switch the timezone. |
I added that as an optional argument to |
|
||
public static Value format(Starscript ss, int argCount) { | ||
if (argCount < 1) ss.error("format(fmt, ...args) requires at least 1 argument, got %d.", argCount); | ||
ArrayList<Object> args = new ArrayList<Object>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current implementation is less than ideal in performance
- using the no-arg
ArrayList
constructor means this method will potentially cause several object arrays to be allocated as the list will have to be resized - inserting objects at index 0 means that the other elements will have to be offset constantly
it would be better to just use an object array directly and filling it with a reverse for i loop
case String: o = v.getString(); break; | ||
case Function: o = v.getFunction(); break; | ||
case Map: o = v.getMap(); break; | ||
case Null: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of initializing Object o
as null
and simply breaking out of the switch on case Null
/default
, it would be more readable if o
wasn't initialized to a value, but case Null
/default
set it to null
Fixed. Any more suggestions? |
This commit adds
format
andformatDateTime
to Starscript. Yes, I know one of the selling points of Starscript is to be faster thanString.format
, but sometimes you do need more formatting options.format(fmt, ...args)
Adds arbitrary formatting by calling
String.format
. This can be considered as a fix to [#7] too.formatDateTime(fmt)
Calls
SimpleDateFormat
because currently Starscriptdate
andtime
don't allow the user to choose a custom format.