-
Notifications
You must be signed in to change notification settings - Fork 554
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
Self-labeling #1444
Self-labeling #1444
Conversation
Generators and Lists are set to have labels added in the future. We can hold off adding self-labels to them until they get labeling added in general. |
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.
Neat :3
Excited to see these go in! Hopefully some thoughts/comments are alright; thanks for your time and hard work!
labels?: | ||
| ComAtprotoLabelDefs.SelfLabels | ||
| { $type: string; [k: string]: unknown } |
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.
💭 Given the repetition of this type in multiple places, could be a good idea to make a named type somewhere (also could make it easier in the future to refine that secondary union type if the need arises)
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.
Could definitely imagine doing that some time, but it's not related to this work so not too worried about it at the moment. These are also created via codegen, so it wont be too hard to update if/when we want to do that.
labels: selfLabels | ||
? { | ||
$type: 'com.atproto.label.defs#selfLabels', | ||
values: selfLabels.map((val) => ({ val })), |
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.
❓ Should this map add a $type
property for each value? Maybe com.atproto.label.defs#selfLabel
?
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.
That isn't required here since the values
field is a ref and not an open union. The labels
field above is an open union, so it need to contain the $type
👍 We think this will provide a decent amount of flexibility without it becoming too verbose.
@@ -268,7 +293,7 @@ export class ActorViews { | |||
following: cur?.requesterFollowing || undefined, | |||
followedBy: cur?.requesterFollowedBy || undefined, | |||
}, | |||
labels: skipLabels ? undefined : actorLabels, | |||
labels: skipLabels ? undefined : [...actorLabels, ...selfLabels], |
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.
💭 It seemed fairly trivial to not avoid aggregating labels before self-labeling was a thing, but now that it is, would be it worth it avoid the work of doing label aggregation unless we explicitly need to?
e.g.
const labels = skipLabels ? undefined : [...(labels[cur.did] ?? []), ... getSelfLabels({
uri: cur.profileUri,
cid: cur.profileCid,
record: cur.profileBytes && cborToLexRecord(cur.profileBytes),
})]
or (maybe cleaner)
// ...earlier
function getLabels(cur) {
const actorLabels = labels[cur.did] ?? []
const selfLabels = getSelfLabels({
uri: cur.profileUri,
cid: cur.profileCid,
record: cur.profileBytes && cborToLexRecord(cur.profileBytes),
})
return [...actorLabels, ...selfLabels];
}
const profile = {
// ... other props
labels: skipLabels ? undefined : getLabels(cur),
// ... rest of props
}
Okay we're ready to go. If yall can publish a new dev-env with this merge that would be helpful! |
Published |
* Add self-label schemas * switch around array and union for self-labels * apply self-labels to post and profile views * test for self labels in pds * apply self-labels to post and profile views in bsky appview * test for self labels in bsky appview * update pds proxied test snapshots * Add support for self-labels to the mdoeration sdk * Disable unknown-labeler filtering until 3P support is added --------- Co-authored-by: Devin Ivy <devinivy@gmail.com>
* Add self-label schemas * switch around array and union for self-labels * apply self-labels to post and profile views * test for self labels in pds * apply self-labels to post and profile views in bsky appview * test for self labels in bsky appview * update pds proxied test snapshots * Add support for self-labels to the mdoeration sdk * Disable unknown-labeler filtering until 3P support is added --------- Co-authored-by: Devin Ivy <devinivy@gmail.com>
Adds "self-labeling," the ability for users to set content warnings on their own content.
GeneratorViewListViewBasicListViewCloses #1403