-
Notifications
You must be signed in to change notification settings - Fork 406
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
Lexicon: richtext facets, replacing post entities #674
Conversation
@@ -12,8 +12,13 @@ | |||
"text": {"type": "string", "maxLength": 256}, | |||
"entities": { | |||
"type": "array", | |||
"description": "Deprecated: replaced by app.bsky.richtext.facet.", |
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.
"description": "Deprecated: replaced by app.bsky.richtext.facet.", | |
"description": "Deprecated: replaced by facets.", |
"description": "A facet value for links.", | ||
"required": ["uri"], | ||
"properties": { | ||
"uri": {"type": "string"} |
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.
Minor note: in #587 I think the intention was to use a uri format here, but we decided to just have the at-uri format for now, which wouldn't apply here.
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.
actually I'd be down to throw in a uri format 🤷♂️
import { Kysely } from 'kysely' | ||
|
||
export async function up(db: Kysely<unknown>): Promise<void> { | ||
await db.schema.alterTable('post_entity').renameTo('post_facet').execute() |
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 remember this table is missing a primary key. Would be a good time to get one on it
But also: I don't think we actually use this table for anything. Maybe we could drop??
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.
Yeah good call, let's just drop this bad boy.
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 👍
Just the question of primary key or drop table. I lean towards drop unless you see a reason to keep it around 👀
This deprecates the post entities system and introduces richtext facets through
app.bsky.richtext.facet
.Resolves #587