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.

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

Implement iterator for all assigned Unicode Blocks #227

Merged
merged 9 commits into from
Oct 11, 2018
Merged

Implement iterator for all assigned Unicode Blocks #227

merged 9 commits into from
Oct 11, 2018

Conversation

eyeplum
Copy link
Member

@eyeplum eyeplum commented Aug 1, 2018

This pull request partially implements #206 by implementing a block iterator for all assigned Unicode Blocks.

@eyeplum
Copy link
Member Author

eyeplum commented Aug 1, 2018 •
edited
Loading

Hey @behnam I know you were working on #206 , but I needed this in my project so I also tried to implement this.

Feel free to close this PR if it can be covered by your work, or just take the useful parts.

Cheers!

@behnam behnam self-requested a review August 20, 2018 06:08
behnam
behnam requested changes Aug 20, 2018
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.

Awesome, @eyeplum! Could you please rebase so we can see if there are any issues needing attention?

@eyeplum
Copy link
Member Author

eyeplum commented Aug 20, 2018

Thanks @behnam , I will rebase and update the PR!

@LuoZijun LuoZijun mentioned this pull request Sep 9, 2018
Closed
behnam
behnam reviewed Oct 9, 2018
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.

Awesome! Mostly looks good, one important question inline, though...

unic/ucd/block/tests/basic_tests.rs Show resolved Hide resolved
unic/ucd/block/src/block.rs Outdated

fn new(raw_block: Option<(CharRange, &'static str)>) -> Option<Block> {
match raw_block {
None => None,
Copy link
Member

Choose a reason for hiding this comment

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

Hum... why do we need to be able to call new() with None and receive None. It's kind of counter-intuitive to Rust's new() pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I think I was trying optimize for the two use cases in this file which both provide an Option<(CharRange, &'static str)> and expect an Option<Block>.
Should be fixed now.
I have also added some comments about the missing surrogate code point blocks.

@behnam
Copy link
Member

behnam commented Oct 9, 2018

Awesome! Looks great!

bors: r+

bors bot added a commit that referenced this pull request Oct 9, 2018
@bors @eyeplum
Merge #227
fa7e8fc 
227: Implement iterator for all assigned Unicode Blocks r=behnam a=eyeplum

This pull request partially implements #206 by implementing a block iterator for all assigned Unicode Blocks.

Co-authored-by: Yan Li <eyeplum@gmail.com>
@behnam
Copy link
Member

behnam commented Oct 9, 2018

Oh, we need to fix master on beta/nightly first. A bunch of non-top-level use statements need a create:: prefix. (See https://travis-ci.org/open-i18n/rust-unic/jobs/439036265) Would be great if you could help with it. If not, I'll try to get to it later this week.

@bors
Copy link
Contributor

bors bot commented Oct 9, 2018

Build failed

@eyeplum
Copy link
Member Author

eyeplum commented Oct 9, 2018

Sure, I'm keen to help. Will send pull request shortly.

@eyeplum
Copy link
Member Author

eyeplum commented Oct 10, 2018

Hey @behnam, I tried to fix those warnings for beta and nightly in #233, but looks like the preferred way is not supported by stable and 1.26.0, so I'm afraid we can't fix those warnings at the moment.
The build failure in this pull request turns out to be some outdated lint annotations in ucd/block's lib.rs (I copied it from some other module at some point). I have aligned ucd/block's lint annotations with other modules, now this pull request should build 🤞.

@behnam
Copy link
Member

behnam commented Oct 10, 2018

Copying from the other PR (#233):

Oh, I thought master is broken. Apparently it's not, but the builds here were failing because of the new code was based on the code before we allow warnings again.

So, let's just crate:: prefixes and all should be fine now. Right?

@behnam behnam mentioned this pull request Oct 10, 2018
Closed
@eyeplum
Copy link
Member Author

eyeplum commented Oct 10, 2018

but the builds here were failing because of the new code was based on the code before we allow warnings again

Exactly, those build errors were actually warnings to warn us about that new compilers will drop support for top level use statements without prefixes (because of #![forbid(future_incompatible)]).

So, let's just crate:: prefixes and all should be fine now. Right?

The crate:: prefixes are not supported by stable and 1.26.0, so we can't do this now.
What I have done in 746a58d is to make future_incompatible a warning instead of an error.

And yes, now this pull request is good to merge (it passed all Travis CI and AppVeyor checks).

@behnam
Copy link
Member

behnam commented Oct 11, 2018

bors: r+

bors bot added a commit that referenced this pull request Oct 11, 2018
@bors @eyeplum
Merge #227
1378b2a 
227: Implement iterator for all assigned Unicode Blocks r=behnam a=eyeplum

This pull request partially implements #206 by implementing a block iterator for all assigned Unicode Blocks.

Co-authored-by: Yan Li <eyeplum@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 11, 2018

Build succeeded

@bors bors bot merged commit 746a58d into open-i18n:master Oct 11, 2018
@eyeplum eyeplum deleted the ucd/block branch January 9, 2019 07:16
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
@eyeplum @behnam