Skip to content

char_property binary property support #155

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Sep 20, 2017
Merged

char_property binary property support #155

merged 8 commits into from
Sep 20, 2017

Conversation

CAD97
Copy link
Collaborator

@CAD97 CAD97 commented Sep 20, 2017 •
edited
Loading

Update to #154. See #154's review before the creation of this PR for a lot of my reasoning.

Here is a direct diff of the two branches.

CAD97 and others added 7 commits September 18, 2017 21:24
@CAD97 @behnam
Add experimental binary property support to char_property!
9462b33
@CAD97 @behnam
Reduce duplication and flush out test
1541f8e
@CAD97 @behnam
Let this work before 1.20
d3db6d8 
This should be reverted when the minimum supported version bumps to 1.20.
@behnam
[ucd/bidi] Use char_property macro for BidiMirrored
0095324 
* Update `char_property!()` to use `data_table_path` option instead of
`mod data`.

* Add doc block to `pub fn`s.

* Update `BidiMirrored` implementation to use `char_property` macro.
@behnam
[char/property] Add more tests and some clean up
fc00f99
@CAD97
Apply my design review realizations from behnam#154
77a3331
@CAD97
Use char_property! for BidiControl
42201bb
@CAD97 CAD97 requested a review from behnam September 20, 2017 03:25
@CAD97
Copy link
Collaborator Author

CAD97 commented Sep 20, 2017

Oh right, there was a change sometime between 1.17 and now that changed the behavior of include! in doctests. In current Rust, it's relative to where it's written. In 1.17, I believe it's relative to the root of the project.

Ugh. The fix is to reignore the binary property doctest, but that just makes me sad.

See rust-lang/rust#34431

@behnam
Copy link
Member

behnam commented Sep 20, 2017

So, @CAD97, should we land this first, and I rebase the left-over matters on top of this? Want to fix the error, then?

behnam
behnam approved these changes Sep 20, 2017
View reviewed changes
Copy link
Member

@behnam behnam left a comment

Choose a reason for hiding this comment

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

Thanks, @CAD97. This is a good step to get all the stable parts in. #154 has some stuff to drop, and some to work on.

@behnam
Copy link
Member

behnam commented Sep 20, 2017

By the way, about the tables dir, for when it's part of the package (library) source, it goes under tables/ in package root. When it's only used for tests, it goes under tests/tables/. This is similar to how we organize the source data, just to make it clear what data is used in library and what's used in dev/test.

This came up about in another PR and I forgot to answer there.

And, here, unic/char/property/tables/property_table.rsv should be in unic/char/property/tests/tables/property_table.rsv.

@behnam behnam mentioned this pull request Sep 20, 2017
Merged
@bors bors bot merged commit a4c2ddb into open-i18n:master Sep 20, 2017
@CAD97 CAD97 deleted the binary-prop branch September 20, 2017 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behnam behnam behnam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CAD97 @behnam