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: bb8c68b 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 ↗︎
5 Skipped Deployments
|
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!
packages/namerank-sdk/src/index.ts
Outdated
| @@ -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.
There was a problem hiding this comment.
@djstrong Ah, I see. Am I correct to assume that these ideas are actually related to the LabelInspector? Assuming so, what do you think about creating a separate labelinspector.ts file that you could move those definitions into, and then importing those definitions back into this file.
My goal here is to keep this file more "pure" to NameRank.
I don't suggest we create a separate typescript package for a LabelInspector SDK in this PR. This would be a nice goal for a separate PR, where we might move this labelinspector.ts file into a separate package, but for now just creating a new and separate file in this current package is ok 👍
Appreciate your advice!
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
There was a problem hiding this comment.
@djstrong Hey super updates! Thank you. Reviewed and shared feedback 👍
packages/namerank-sdk/src/index.ts
Outdated
| export interface NLPLabelAnalysis { | ||
| inspection: any; | ||
| /** The result of the label inspection */ |
There was a problem hiding this comment.
Could we also document here that inspection will be of type InspectorResultNormalized if status is Normalized and otherwise will be of type InspectorResultUnnormalized?
What if status is Unknown?
packages/namerank-sdk/src/index.ts
Outdated
| interesting_score: number; | ||
|
|
||
| /** | ||
| * The result of the NLP analysis on the label. If the label is not inspected, this field will be undefined. |
There was a problem hiding this comment.
Could we refine this to make it very explicit what it means for the label not to be inspected? What field / field value would define this case?
packages/namerank-sdk/src/index.ts
Outdated
| @@ -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.
@djstrong Ah, I see. Am I correct to assume that these ideas are actually related to the LabelInspector? Assuming so, what do you think about creating a separate labelinspector.ts file that you could move those definitions into, and then importing those definitions back into this file.
My goal here is to keep this file more "pure" to NameRank.
I don't suggest we create a separate typescript package for a LabelInspector SDK in this PR. This would be a nice goal for a separate PR, where we might move this labelinspector.ts file into a separate package, but for now just creating a new and separate file in this current package is ok 👍
Appreciate your advice!
Co-authored-by: lightwalker.eth <[email protected]>
Co-authored-by: lightwalker.eth <[email protected]>
lightwalker-eth
deleted the
jakubkarbowski/sc-25972/refine-namerank-sdk
branch
Story details: https://app.shortcut.com/ps-web3/story/25972