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

bidi rules #497

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

bidi rules #497

wants to merge 1 commit into from

Conversation

rtf-const
Copy link

@rtf-const rtf-const commented May 7, 2019

This is a pull request for issue #489


This change is Reviewable

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #517) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link

@vorner vorner left a comment

Choose a reason for hiding this comment

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

I've promised to find the time to do the review here.

I've tried to read it thoroughly and I believe the changes do what you say in the comment, with some very minor nitpicking and questions.

That doesn't answer the question if this is the behaviour all the standards agree on, though. I'll find the time to try reading through the standards mentioned there later and try to come up with an answer.

@@ -37,6 +40,13 @@ pub fn collect_tests<F: FnMut(String, TestFn)>(add_test: &mut F) {
let to_ascii = pieces.remove(0);
let nv8 = if pieces.len() > 0 { pieces.remove(0) } else { "" };

for skip in SKIP_TEST {
if original == *skip {
Copy link

Choose a reason for hiding this comment

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

Can this be done with SKIP_TEST.contains(original)?

@@ -10,6 +10,9 @@ use std::char;
use idna::uts46;
use test::TestFn;

const SKIP_TEST: &'static [&'static str] = &["0A.\\u05D0", "0a.\\u05D0", "0a.xn--4db"
,"1.걾6.𐱁\\u06D0", "1.걾6.𐱁\\u06D0","1.xn--6-945e.xn--glb1794k"];
Copy link

Choose a reason for hiding this comment

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

Probably explaining why these are skipped in a comment would be nice ‒ maybe linking to the issue?

}
_ => { break; }
}
}
Copy link

Choose a reason for hiding this comment

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

This loop doesn't really check if it is truly a LDH label. I think it would accept something like 3_4 as well (_ is ascii, but neither letter, digit or hyphen). Does something else check that in the existing code?

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

3 participants