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

feat: remove Js.String2, use labeled arguments for Js.String #893

Merged
merged 6 commits into from
Nov 16, 2023

Conversation

anmonteiro
Copy link
Member

@anmonteiro anmonteiro commented Nov 15, 2023

part of the unification proposed in #731

this needs the melange v3 long-lived branch in reason-react to compile the template projects.

return {
TAG: /* Eq */0,
_0: [
"a",
"#",
undefined
],
_1: "a#b#:c".split(/(#)(:)?/, 3)
_1: (function (param) {
Copy link
Member Author

Choose a reason for hiding this comment

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

expected since the test uses |>.

@anmonteiro
Copy link
Member Author

Before we merge this, we should add a test for string interpolation that fails in the current PR state. There's still a mention of String2 in the ppx:

melange/ppx/utf8_string.ml

Lines 463 to 464 in ac03e24

let to_string_ident : Longident.t =
Ldot (Ldot (Lident "Js", "String2"), "make")

Copy link
Member

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Nice! I didn't go through all cases where a simplification can be applied, as I want to know your thoughts about that suggestion first.

jscomp/runtime/js_string.ml Outdated Show resolved Hide resolved
*)

(** [codePointAt n s] returns the code point at position [n] within string [s] as a [Some] value. The return value handles code points greater than or equal to [0x10000]. If there is no code point at the given position, the function returns [None].
external codePointAt : t -> pos:int -> int option = "codePointAt"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I used ~pos because that's what OCaml's StdLabels uses.

jscomp/runtime/js_string.ml Outdated Show resolved Hide resolved
jscomp/runtime/js_string.ml Outdated Show resolved Hide resolved
jscomp/runtime/js_string.ml Outdated Show resolved Hide resolved
jscomp/runtime/js_string.ml Show resolved Hide resolved
jscomp/runtime/js_string.ml Outdated Show resolved Hide resolved
jscomp/runtime/js_string.ml Outdated Show resolved Hide resolved
jscomp/runtime/js_string.ml Outdated Show resolved Hide resolved
@anmonteiro
Copy link
Member Author

I didn't go through all cases where a simplification can be applied, as I want to know your thoughts about that suggestion first.

I like your suggestion. Check my reply in #895 (comment)

@anmonteiro anmonteiro merged commit 3a4b2b1 into main Nov 16, 2023
4 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/migrate-string2 branch November 16, 2023 22:55
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.

None yet

2 participants