feat: [sc-25972] Refine NameRank SDK #515
Conversation
Carbon225
requested review from
djstrong,
notrab,
lightwalker-eth,
FrancoAguzzi,
BanaSeba and
eduramme
as code owners
🦋 Changeset detectedLatest commit: 8c0a4b4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
@Carbon225 Hey great to see the updates here. Reviewed in detail and shared feedback 👍 @djstrong FYI
packages/namerank-sdk/src/index.ts
Outdated
| word_count: number; | ||
|
|
||
| /** The most likely tokenization of the label */ |
There was a problem hiding this comment.
Thanks. Can we please document in detail the rules for if this field is defined or not? This will be very helpful!
| @@ -9,24 +9,345 @@ export enum LabelStatus { | |||
| Unknown = "unknown", | |||
| } | |||
|
|
|||
| // Character and grapheme types can potentially evolve independently. | |||
|
|
|||
| export enum CharacterType { | |||
There was a problem hiding this comment.
@Carbon225 It's great to see the added documentation! In my understanding, we should already have most or all of these defined and documented in the NameGuard SDK? We shouldn't duplicate these definitions here. Instead, we should import from the NameGuard SDK wherever relevant. Thank you! @djstrong FYI
There was a problem hiding this comment.
None of these are present in NameGuard SDK.
packages/namerank-sdk/src/index.ts
Outdated
| interesting_score: number; | ||
|
|
||
| /** Detailed NLP analysis of the name */ |
There was a problem hiding this comment.
Can we please document what can cause this field to be undefined?
packages/namerank-sdk/src/index.ts
Outdated
| tokenizations: Record<string, any>[]; | ||
| } | ||
|
|
||
| export interface NameRankReport { | ||
| /** Score indicating the purity/cleanliness of the name */ |
There was a problem hiding this comment.
Can we please document the range of values this could be? Ex: 0 to 1? Inclusive / exclusive?
packages/namerank-sdk/src/index.ts
Outdated
| purity_score: number; | ||
|
|
||
| /** Score indicating how interesting/memorable the name is */ |
There was a problem hiding this comment.
Can we please document the range of values this could be? Ex: 0 to 1? Inclusive / exclusive?
packages/namerank-sdk/src/index.ts
Outdated
| top_tokenization?: string[]; | ||
|
|
||
| /** All possible tokenizations of the label */ |
There was a problem hiding this comment.
Is it really true that this will always have all possible tokenizations? I imagine there's some kind of a limit to how many we might generate / return?
packages/namerank-sdk/src/index.ts
Outdated
| status: LabelStatus; | ||
|
|
||
| /** The probability of the label */ |
There was a problem hiding this comment.
Let's please document in a bit more detail what this "probability" is intended to symbolically represent.
packages/namerank-sdk/src/index.ts
Outdated
| log_probability: number; | ||
|
|
||
| /** The number of words in the label */ |
There was a problem hiding this comment.
Can we please document more details about this word_count field? For example, is it sometimes 0 or undefined? What happens if we can't find any words? And when we do return a positive word_count value, how does this value relate to the top_tokenization field?
packages/namerank-sdk/src/index.ts
Outdated
| top_tokenization?: string[]; | ||
|
|
||
| /** All possible tokenizations of the label */ | ||
| tokenizations: Record<string, any>[]; |
There was a problem hiding this comment.
Let's please also define an interface for the value of each of these records. We should avoid the use of any.
djstrong
force-pushed
the
jakubkarbowski/sc-25972/refine-namerank-sdk
branch
from
4164e5b to
74488e8
Compare
|
@lightwalker-eth I have addressed all issues. |
|
@djstrong @lightwalker-eth we should add a changesets entry so the types are released as a minor update. |
Story details: https://app.shortcut.com/ps-web3/story/25972