Skip to content

Add js.lib.NativeStringTools #8633

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

terurou
Copy link
Member

@terurou terurou commented Aug 14, 2019

No description provided.

the same as the given string in sort order.
*/
public static inline function localeCompare(string:String, compareString:String, ?locales:EitherType<String, Array<String>>, ?options:CollatorOptions):Bool {
return if (locales == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing a branch for a case when both locales and options are null.

Copy link
Member Author

@terurou terurou Aug 14, 2019

Choose a reason for hiding this comment

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

Thanks for your review! I fixed it.
localeCompare() throws an error when it takes locales = null. So I use undefined.

@RealyUniqueName RealyUniqueName added this to the Release 4.0 milestone Aug 21, 2019
@RealyUniqueName
Copy link
Member

@haxiomic could you please check this?

@haxiomic
Copy link
Member

haxiomic commented Aug 22, 2019

To avoid the need to fill out different cases for argument combinations we can use @:native to the String.prototype methods

For example

@:noClosure
@:native('')
extern class NativeStringTools {

	/**
		Returns a number indicating whether a reference string comes before or after or is
		the same as the given string in sort order.
	 */
	@:native('String.prototype.localeCompare.call')
	public static function localeCompare(string: String, compareString: String, ?locales: EitherType<String, Array<String>>, ?options: CollatorOptions):Bool;
}
using js.lib.NativeStringTools;

trace('a'.localeCompare('b'));

// String.prototype.localeCompare.call("a","b")

I'm not sure if there's a cleaner approach – what do you think @nadako?

@RealyUniqueName RealyUniqueName modified the milestones: Release 4.0, Backlog Sep 1, 2019
@haxiomic haxiomic mentioned this pull request Sep 3, 2019
@terurou
Copy link
Member Author

terurou commented Sep 5, 2019

ping @nadako @haxiomic
related: #8731 (comment)

@haxiomic
Copy link
Member

haxiomic commented Sep 5, 2019

@terurou I'm looking more at that @:native change, the nulls appearing actually looks to be a bug with calling extern methods within inlined methods

It applies to all externs with @:native or not, the workaround in the short term is to remove inline from abstract BigInt.toLocaleString and others

(nadako is away atm)

@haxiomic
Copy link
Member

haxiomic commented Sep 5, 2019

But if you want to keep the null check approach, then maybe something like

    Syntax.code(
        "({0}).toLocaleString()",
        this,
        locales == null ? js.Lib.undefined : locales,
        options == null ? js.Lib.undefined : options
    );

Is more readable and maintainable

@terurou
Copy link
Member Author

terurou commented Sep 5, 2019

I don't like "prototype call style" because Haxe compiler emits null into optional parameter now. In JavaScript, null and undefined are different type 👎

I think this style is better:

    Syntax.code(
        "({0}).toLocaleString()",
        this,
        locales == null ? js.Lib.undefined : locales,
        options == null ? js.Lib.undefined : options
    );

terurou added a commit to terurou/haxe that referenced this pull request Sep 10, 2019
@Simn Simn modified the milestones: Backlog, Later Mar 24, 2023
@Simn
Copy link
Member

Simn commented Apr 6, 2025

@kLabz Could you check if we can merge this with #12127?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants