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

Rename CodePointSet to CodePointInversionList #2230

Merged
merged 9 commits into from
Jul 26, 2022

Conversation

echeran
Copy link
Contributor

@echeran echeran commented Jul 23, 2022

Fixes #2225

@echeran echeran marked this pull request as ready for review July 23, 2022 01:38
@echeran echeran removed request for a team, hsivonen and robertbastian July 23, 2022 01:39
Manishearth
Manishearth previously approved these changes Jul 25, 2022
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.

Note that this may conflict with #2235; since @hsivonen is OOO, I would prefer if we merge #2235 first and then fix the merge conflict in this PR.

represented by [inversion lists](http://userguide.icu-project.org/strings/properties),
the [`CodePointSetBuilder`], or from the TBA Properties API.
the [`CodePointInversionListBuilder`], or from the TBA Properties API.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The properties API is no longer "TBA"; fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

/// Returns an owned inversion list representing the current [`CodePointSet`]
/// Returns an owned inversion list representing the current [`CodePointInversionList`]
pub fn get_inversion_list(&self) -> Vec<u32> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: name this get_inversion_list_vec() since the type is now called an "inversion list" too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

(setting review bit appropriately)

Manishearth
Manishearth previously approved these changes Jul 25, 2022
sffc
sffc previously approved these changes Jul 25, 2022
@sffc
Copy link
Member

sffc commented Jul 25, 2022

test-docs is unhappy

failures:

---- src/uniset.rs - uniset::CodePointInversionList::all (line 221) stdout ----
error[E0599]: no method named `get_inversion_list` found for struct `CodePointInversionList` in the current scope
 --> src/uniset.rs:226:42
  |
8 | assert_eq!(CodePointInversionList::all().get_inversion_list(), expected);
  |                                          ^^^^^^^^^^^^^^^^^^ help: there is an associated function with a similar name: `get_inversion_list_vec`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
Couldn't compile the test.
---- src/uniset.rs - uniset::CodePointInversionList::bmp (line 245) stdout ----
error[E0599]: no method named `get_inversion_list` found for struct `CodePointInversionList` in the current scope
  --> src/uniset.rs:252:42
   |
10 | assert_eq!(CodePointInversionList::bmp().get_inversion_list(), expected);
   |                                          ^^^^^^^^^^^^^^^^^^ help: there is an associated function with a similar name: `get_inversion_list_vec`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
Couldn't compile the test.

failures:
    src/uniset.rs - uniset::CodePointInversionList::all (line 221)
    src/uniset.rs - uniset::CodePointInversionList::bmp (line 245)

test result: FAILED. 30 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.10s

@echeran echeran dismissed stale reviews from sffc and Manishearth via 4ce5de0 July 25, 2022 23:10
@echeran echeran requested a review from sffc July 26, 2022 00:00
@echeran echeran merged commit 354fe3b into unicode-org:main Jul 26, 2022
@echeran echeran deleted the rename-codepointset branch July 26, 2022 00:23
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.

Rename CodePointSet to CodePointInversionList
3 participants