Skip to content

Conversation

miguelteixeiraa
Copy link
Contributor

ref: #222

The good news is that almost all tests cases are passing.

@lemire
Copy link
Member

lemire commented Feb 27, 2023

@miguelteixeiraa Would you add it to https://github.com/ada-url/idna ?

We eventually want to rely on ada-url/idna for ada.

@miguelteixeiraa
Copy link
Contributor Author

Makes sense, good idea!

@miguelteixeiraa
Copy link
Contributor Author

@miguelteixeiraa Would you add it to https://github.com/ada-url/idna ?

We eventually want to rely on ada-url/idna for ada.

Done!

@lemire
Copy link
Member

lemire commented Mar 4, 2023

@miguelteixeiraa Why did you close it here? In my view, it belongs at both places. Your work here was good and relevant. Of course, here you were testing our ICU-based implementation, but no matter. My understanding is that your tests ought to pass everywhere (or, else, we need to figure something out).

@anonrig
Copy link
Member

anonrig commented Mar 8, 2023

There is 1 test case we are failing at the moment.

FAIL: 
  Returned: xn--8-qfa.xn---?-261a
  Test case: {
    "comment": "P1; V6; V3 (ignored)",
    "input": "8\u00df.\udb40\udd10-?\u2d0f",
    "output": null
  }

@anonrig
Copy link
Member

anonrig commented Mar 8, 2023

We moved is_forbidden_domain_code_point check to outside of to_ascii method. I assume this is why it's breaking this pull request. We just need to add the following lines to wpt_tests with a TODO to fix this when we get rid of ICU.

if (output.has_value()) {
  if (std::any_of(output.value().begin(), output.value().end(), ada::unicode::is_forbidden_domain_code_point)) {
    output = std::nullopt;
  }
}

@miguelteixeiraa
Copy link
Contributor Author

Is it okay to remove the ada_really_inline constexpr from is_forbidden_domain_code_point? I'm getting linkage problems.

@lemire
Copy link
Member

lemire commented Mar 28, 2023

@miguelteixeiraa This will need to be updated because we did major surgery on the code.

@lemire
Copy link
Member

lemire commented Mar 28, 2023

Let me see if I can integrate the code in the other PR.

@lemire
Copy link
Member

lemire commented Mar 28, 2023

I am going to push the content of this PR into #216

It will be less trouble for everyone.

You may close this PR.

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.

3 participants