Refactor properties to separate crate #1153

echeran · 2021-10-06T16:38:52Z

Renaming the UnicodePropertyV1 data struct, which represents UnicodeSet data for binary properties and for enumerated properties (per key=val pair). It would allow the corresponding data provider struct for CodePointTrie data, which would only exist for enumerated properties, to have a consistent name, once created.

iainireland · 2021-10-06T17:27:14Z

The "Data" in "UnicodeSetData" doesn't seem necessary. (Compare, say icu_decimal::provider, where we have AffixesV1, GroupingSizesV1, and so on.) Alternative suggestions: UnicodeSetV1, UnicodePropertyV1, UnicodePropertySetV1?

What do you intend to call the CodePointTrie equivalent?

echeran · 2021-10-08T20:49:13Z

The "Data" in "UnicodeSetData" doesn't seem necessary. (Compare, say icu_decimal::provider, where we have AffixesV1, GroupingSizesV1, and so on.) Alternative suggestions: UnicodeSetV1, UnicodePropertyV1, UnicodePropertySetV1?

What do you intend to call the CodePointTrie equivalent?

I looked at other data provider structs, and it seems like they all have a plural noun name, not a singular word name -- just an observation.

As far as how to re-/name the data struct for UnicodeSet data in a way that will be consistent with the data struct for CPT data, I'm open to suggestions for improve my naming ideas. We currently have UnicodePropertyV1 and UnicodePropertyMapV1, but ythen ou could imagine changing UnicodePropertyV1 -> UnicodePropertySetV1 for consistency. But then at that point, why not drop the "Property" -- b/c I think the distinguishing factor between these data structs is the shape of the data (the data structure) that they contain -- and then you get UnicodeSetV1 and UnicodeMapV1.

I thought UnicodeSetV1 sounded too much like UnicodeSet itself (which is why I originally said UnicodeSetDataV1), but that's enough naming bikeshedding for me, and I usually defer to others for short better names.

jira-pull-request-webhook · 2021-10-13T01:09:10Z

Notice: the branch changed across the force-push!

Cargo.lock is now changed in the branch
Cargo.toml is now changed in the branch
components/uniset/Cargo.toml is now changed in the branch
components/uniset/src/lib.rs is now changed in the branch
components/uniset/src/props.rs is different
components/uniset/src/provider.rs is no longer changed in the branch
docs/tutorials/writing_a_new_data_struct.md is no longer changed in the branch
provider/uprops/src/binary.rs is no longer changed in the branch
provider/uprops/src/enumerated.rs is no longer changed in the branch
provider/uprops/src/provider.rs is no longer changed in the branch
utils/properties/Cargo.toml is now changed in the branch
utils/properties/README.md is now changed in the branch
utils/properties/src/enum_props.rs is now changed in the branch
utils/properties/src/lib.rs is now changed in the branch
utils/properties/src/ule.rs is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

echeran · 2021-10-13T01:21:17Z

The conversation above is relevant to PR #1167, and I incorporated those ideas into that PR since it seemed necessary to avoid confusing code and naming.

However, in order for that PR to proceed, it needs to solve a confusing and misleading circular dependency that is currently caused by having property enums / newtype structs defined in icu_uniset, but the TrieValue trait in icu_codepointtrie. We intend to implement TrieValue for the property enums/newtypes.

At the least, we should refactor the property enums/newtypes to a separate crate. This PR is being renamed and re-purposed to represent that new scope of work which will unblock #1167.

(And if Rust only allows impls of traits on enums/newtypes in the same module/crate in which they are defined, then perhaps the TrieValue trait should be moved to this new crate, too?)

iainireland · 2021-10-13T01:41:33Z

Regarding names: I don't have strong feelings, but it occurs to me that if we were doing this from scratch, without basing anything on ICU4C, I think it's likely that we'd omit "Unicode" and call these something more like "CharacterSet" and "CharacterMap". Most of our data is from Unicode; what's interesting about this data is that it represents character properties.

sffc

Please make sure all the points from the renaming issue are covered

sffc · 2021-10-18T22:38:14Z

components/properties/src/enum_props.rs

-pub struct GeneralCategory(pub(crate) u32);
+pub struct GeneralCategory(pub u32);


Please revert this change, and if you really need it, add a impl TryFrom<u32> for GeneralCategory if there isn't already such an impl. @iainireland thoughts? Don't we already need this for the Serde?

sffc · 2021-10-18T22:41:10Z

components/properties/src/enum_props.rs

-pub struct Script(pub(crate) u16);
+pub struct Script(pub u16);


sffc · 2021-10-18T22:41:28Z

components/properties/src/lib.rs

+//! `icu_properties` is a utility crate of the [`ICU4X`] project.
+//!
+//! This component provides definitions of [Unicode Properties].
+//!
+//! [Unicode Properties]: https://unicode-org.github.io/icu/userguide/strings/properties.html


Nit: Please strengthen these docs

sffc · 2021-10-18T22:42:12Z

components/properties/src/lib.rs

+mod enum_props;
+mod ule;
+
+pub use enum_props::*;


Discussion: Do we want to pub use enum_props::* and put everything in the top-level namespace?

I see two reasonable choices:

icu_properties::get_alnum() and icu_properties::get_general_category()

icu_properties::sets::get_alnum() and icu_properties::maps::get_general_category()

I slightly favor hoisting to the top level (1/10), but I can see advantages to the smaller namespaces.

@zbraniecki says:

my intuition is that it's easier to consoidate namespaces than separate them, so I'd start with more specific (::sets::) and reconsider once the API surface stabilizes

So I think we should go ahead with the smaller namespaces for now, and revisit this later when we do a comprehensive API review prior to 1.0. (We don't need a separate issue to track that)

Done. We only have getter APIs for UnicodeSets, which are in the new icu_properties::sets module. The enums/newtypes for the actual properties themselves are up in the top-level namespace.

sffc · 2021-10-19T01:29:38Z

components/icu/src/lib.rs

-    pub use icu_uniset::*;
+    pub use icu_properties::*;


Please rename icu::uniset to icu::properties.

This line was part of the section defining icu::uniset, which was removed.

sffc · 2021-10-19T01:30:20Z

utils/uniset/src/builder.rs

-    /// use icu::uniset::UnicodeSetBuilder;
+    /// use icu_uniset::UnicodeSetBuilder;


These should still be icu::uniset, pending the discussion below.

Because we decided to remove icu::uniset, I am keeping it was icu_uniset because we cannot revert back to icu::uniset.

components/icu/src/lib.rs

… property enums+etc. module (`enum_props`->`props`)

… separate crate)

iainireland · 2021-10-19T23:13:18Z

components/properties/src/props.rs

+/// GeneralSubcategory only supports specific subcategories (eg `UppercaseLetter`).
+/// It does not support grouped categories (eg `Letter`). For grouped categories, use [`GeneralCategory`].


Praise: good catch

sffc · 2021-10-19T23:59:13Z

components/icu/src/lib.rs

+    //! [`ICU4X`]: ../icu/index.html
+    //! [Unicode Properties]: https://unicode-org.github.io/icu/userguide/strings/properties.html
+    //! [`UnicodeSet`]: ../../icu_uniset/struct.UnicodeSet.html
+    //! [`sets`]: sets


Here and below: Let's fix these crossrefs.

sffc · 2021-10-20T00:08:12Z

components/properties/src/lib.rs

+#![no_std]
+
+mod props;
+#[allow(unused)]


Why do you have the #[allow(unused)] ?

sffc · 2021-10-20T00:09:44Z

components/properties/src/sets.rs

@@ -658,7 +659,7 @@ where
        GeneralCategory::SpaceSeparator => key::GENERAL_CATEGORY_SPACE_SEPARATOR_V1,
        _ => return Err(UnicodeSetError::UnknownGeneralCategorySet(enum_val.0)),
    };
-    get_prop(provider, key)
+    get_uniset(provider, key)
 }

 /// Return a [`UnicodeSet`] for a particular value of the Script Unicode enumerated property


Nit: change this function to get_for_script so that you call it as icu::properties::sets::get_for_script()

echeran linked an issue Oct 7, 2021 that may be closed by this pull request

Refactor property definitions to a separate crate #1160

Closed

echeran mentioned this pull request Oct 11, 2021

CodePointTrie data provider #1167

Merged

3 tasks

Move properties enums/structs to separate crate

5b84b60

echeran force-pushed the props-data-refactor branch from e5c4dec to 5b84b60 Compare October 13, 2021 01:09

echeran changed the title ~~Properties data struct renaming~~ Refactor properties to separate crate Oct 13, 2021

echeran linked an issue Oct 18, 2021 that may be closed by this pull request

Crate layout of Unicode Properties #1136

Closed

3 tasks

iainireland mentioned this pull request Oct 18, 2021

Adds UnicodePropertyMapV1 data struct for enumerated properties #1161

Merged

echeran added 3 commits October 18, 2021 14:04

Move properties crate from utils to components

5131cb7

Move uniset from components to utils

fc5cfd5

Add LICENSE file to properties crate in components

75baa55

echeran requested a review from sffc October 18, 2021 21:47

sffc requested changes Oct 18, 2021

View reviewed changes

echeran added 4 commits October 18, 2021 16:53

Move property APIs from icu_uniset to icu_properties

5495357

Revert GeneralCategory arg access to pub(crate)

eba5970

Revert Script arg access to pub(crate)

d67e9fb

Improve documentation for icu_properties and related code.

d039a1b

echeran marked this pull request as ready for review October 19, 2021 01:17

echeran requested review from iainireland and a team as code owners October 19, 2021 01:17

echeran requested a review from sffc October 19, 2021 01:17

sffc reviewed Oct 19, 2021

View reviewed changes

components/icu/src/lib.rs Show resolved Hide resolved

echeran added 3 commits October 18, 2021 18:43

Remove redundant suffix from getters of properties as UnicodeSets

c599983

Rename API getter modules by return type (props->sets) and rename…

eb3fad9

… property enums+etc. module (`enum_props`->`props`)

Remove icu::uniset, replace it with icu::properties

66ac4a6

echeran requested a review from sffc October 19, 2021 17:56

echeran added 3 commits October 19, 2021 10:58

Update Readme file change

1fce90c

Add no_std configuration to icu_properties

4749f57

Update path to uniset crate in CI job for benchmarking

fa17495

echeran mentioned this pull request Oct 19, 2021

Refactor historical perf benchmark data to match uniset source code dir move #1191

Merged

echeran added a commit to echeran/icu4x that referenced this pull request Oct 19, 2021

Merge current snapshot of PR unicode-org#1153 (refactor properties to…

4abb8a4

… separate crate)

iainireland reviewed Oct 19, 2021

View reviewed changes

sffc reviewed Oct 20, 2021

View reviewed changes

Apply review feedback

a598822

echeran requested a review from sffc October 20, 2021 00:35

sffc previously approved these changes Oct 20, 2021

View reviewed changes

Update link to perf benchmark page for icu_uniset

667ab42

echeran dismissed sffc’s stale review via 667ab42 October 20, 2021 00:38

echeran requested a review from sffc October 20, 2021 00:38

sffc approved these changes Oct 20, 2021

View reviewed changes

echeran merged commit 30f0687 into unicode-org:main Oct 20, 2021

echeran deleted the props-data-refactor branch October 20, 2021 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor properties to separate crate #1153

Refactor properties to separate crate #1153

echeran commented Oct 6, 2021

iainireland commented Oct 6, 2021

echeran commented Oct 8, 2021

jira-pull-request-webhook bot commented Oct 13, 2021

echeran commented Oct 13, 2021

iainireland commented Oct 13, 2021

sffc left a comment

sffc Oct 18, 2021

echeran Oct 19, 2021

sffc Oct 18, 2021

echeran Oct 19, 2021

sffc Oct 18, 2021

echeran Oct 19, 2021

sffc Oct 18, 2021

sffc Oct 19, 2021

sffc Oct 19, 2021

echeran Oct 19, 2021

sffc Oct 19, 2021 •

edited

Loading

echeran Oct 19, 2021

sffc Oct 19, 2021 •

edited

Loading

echeran Oct 19, 2021

iainireland Oct 19, 2021

sffc Oct 19, 2021

sffc Oct 20, 2021

sffc Oct 20, 2021

		pub struct GeneralCategory(pub(crate) u32);
		pub struct GeneralCategory(pub u32);

		pub struct Script(pub(crate) u16);
		pub struct Script(pub u16);

		/// use icu::uniset::UnicodeSetBuilder;
		/// use icu_uniset::UnicodeSetBuilder;

		/// GeneralSubcategory only supports specific subcategories (eg `UppercaseLetter`).
		/// It does not support grouped categories (eg `Letter`). For grouped categories, use [`GeneralCategory`].

Refactor properties to separate crate #1153

Refactor properties to separate crate #1153

Conversation

echeran commented Oct 6, 2021

iainireland commented Oct 6, 2021

echeran commented Oct 8, 2021

jira-pull-request-webhook bot commented Oct 13, 2021

echeran commented Oct 13, 2021

iainireland commented Oct 13, 2021

sffc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc Oct 19, 2021 •

edited

Loading

sffc Oct 19, 2021 •

edited

Loading