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

UnicodeSet Builder L2 #195

Merged
merged 45 commits into from
Aug 25, 2020
Merged

UnicodeSet Builder L2 #195

merged 45 commits into from
Aug 25, 2020

Conversation

EvanJP
Copy link
Contributor

@EvanJP EvanJP commented Aug 4, 2020

The L2 Builder API as discussed in #91.

Provides:

  • UnicodeSetBuilder::new()
  • UnicodeSetBuilder::build() -> UnicodeSet
  • add_char, add_range, add_set
  • remove_char, remove_range, remove_set
  • retain_char, retain_range, retain_set
  • complement, complement_char, complement_range, complement_set
  • Cargo Docs

Evan Peng added 30 commits June 3, 2020 21:41
initial import of char_collection
02bd23b
anyhow::Error dependency removed and std::error::Error added
c66b770
std imports made consistent and unic-ucd-block dependency removed
e4b8f56
Replaced CharRange, passing 36/40 tests
4963cc1
Fixed bug, pass all 40 tests
01ca5ef
Remove dependency file
b57d18f
github actions and README fixes
dedebe2
L1 initial completion, unit tests not complete
a1c2d6a
UnicodeSet tests
11bdaec
Complete contains test and docs
dc41bdf
formatting
0a55933
added is_empty() and size()
4a04a96
proposed changes
e8c2b1a
Closure for contains and docs
1471213
Removed unnecessary files and formatting changes
8e2a34a
Update to repo
cc781c8
formatting and cleaning up changes
cfd9edf
replace u32 with char and fix typos and optimizations
c41fa76
remove unnecessary imports, made more rusty
da5eecf
clipply fix
22b4fe0
more clippy lint fixes
8d9138e
Architecture checks minus benchmarks
dac8a4a
added benchmarks and fixed surrogate code points in iter
7fd9082
fix to is_valid
0f5a021
bench changes and other minor fixes
c7c4330
forgot to run fmt
a8a4b50
change to std::char, and unreachable!() optimizations
845bc35
size() is now constant check, ranges() temp removed
dbf3100
fixed bench
a9acfa2
clippy checks that cargo clippy doesn't catch locally
2c54b63
@EvanJP EvanJP self-assigned this Aug 4, 2020
@coveralls
Copy link

coveralls commented Aug 4, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 0582acb9f2970b30bb1669aeb832094b1317df5a-PR-195

  • 445 of 447 (99.55%) changed or added relevant lines in 2 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 91.5%
Changes Missing Coverage Covered Lines Changed/Added Lines %
components/uniset/src/builder.rs 442 444 99.55%
Files with Coverage Reduction New Missed Lines %
components/locale/src/langid.rs 5 0%
Totals Coverage Status
Change from base Build 9a296e8accc5ca174c2cb0432beec34491c266ff: 0.7%
Covered Lines: 1604
Relevant Lines: 1753
💛 - Coveralls

sffc
sffc requested changes Aug 7, 2020
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

A few initial comments

components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
sffc
sffc requested changes Aug 8, 2020
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Great work!

components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Show resolved Hide resolved
components/uniset/src/builder.rs Show resolved Hide resolved
components/uniset/src/uniset.rs Outdated Show resolved Hide resolved
sffc
sffc previously requested changes Aug 10, 2020
View reviewed changes
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
components/uniset/src/builder.rs Outdated Show resolved Hide resolved
sffc
sffc reviewed Aug 10, 2020
View reviewed changes
Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

I think add and remove should be essentially the same algorithm, with just minor differences (e.g., switching even for odd). Can you make the two functions more similar to each other, for example, by making the variable names the same? If there are any differences in business logic, try to figure out why those differences exist and leave comments.

@EvanJP EvanJP assigned EvanJP and unassigned EvanJP Aug 12, 2020
echeran
echeran previously approved these changes Aug 14, 2020
View reviewed changes
zbraniecki
zbraniecki previously approved these changes Aug 18, 2020
View reviewed changes
components/uniset/src/builder.rs Show resolved Hide resolved
@sffc sffc dismissed their stale review August 18, 2020 21:39

Changes are no longer blocking

@sffc
Copy link
Member

sffc commented Aug 18, 2020

I still have an open comment, but otherwise this looks good.

@EvanJP EvanJP dismissed stale reviews from zbraniecki and echeran via b346a56 August 24, 2020 17:05
@EvanJP EvanJP merged commit 862b342 into unicode-org:master Aug 25, 2020
@sffc sffc mentioned this pull request Oct 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sffc sffc sffc left review comments

@echeran echeran echeran approved these changes

@zbraniecki zbraniecki zbraniecki left review comments

@nciric nciric Awaiting requested review from nciric

Assignees

@EvanJP EvanJP

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EvanJP @coveralls @sffc @zbraniecki @echeran