-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add utils for ranking and finding labels #2
Conversation
8568eb4
to
eae5b95
Compare
971c2e4
to
3b6cd6f
Compare
3b6cd6f
to
42f86d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Few small comments && I noticed that test coverage decreased by ~2%. If it's easy enough I'd say shore it up but I'm not super worried about it.
Neither here nor there but I bleh I hate having 2 sources of truth for a thing 🤢 . Let's just hope the python label ranking util doesn't change much, or if it does we're cognizant enough to also update this.
} from "@/arches_vue_utils/types"; | ||
|
||
/* Port of rank_label in arches.app.utils.i18n python module */ | ||
export const rankLabel = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking nit: I've started developing a pretty strong opinion about function declarations vs function expressions. It really doesn't matter in this context but if you've also got strong opinions in general it'd be fun to chat 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
export interface WithLabels { | ||
labels: Label[]; | ||
} | ||
|
||
export interface WithValues { | ||
values: Label[]; | ||
} | ||
|
||
export type Labellable = WithLabels | WithValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not blocking but if these types end up getting used elsewhere I'd say we update them to be a touch more descriptive eg (ItemWithLabels
)
but not too worried about it now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hear you. I don't think they'll get used elsewhere. What's been maddeningly difficult here is that "item" has a technical meaning in the controlled list manager, so I was hoping to avoid that.
Implementation and test cases inspired by
arches.app.utils.i18n
python module