Skip to content
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

[Detach] Lexicons #2664

Merged
merged 7 commits into from
Jul 31, 2024
Merged

[Detach] Lexicons #2664

merged 7 commits into from
Jul 31, 2024

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Jul 25, 2024

Adds a app.bsky.feed.detach lexicon and codegens. targets limit set to 1k for now.

@dholms
Copy link
Collaborator

dholms commented Jul 30, 2024

Snuck in some protos here too 👍

@estrattonbailey estrattonbailey merged commit ff803fd into main Jul 31, 2024
10 checks passed
@estrattonbailey estrattonbailey deleted the detach/lex branch July 31, 2024 21:45
@github-actions github-actions bot mentioned this pull request Jul 31, 2024
Copy link
Collaborator

@bnewbold bnewbold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some comments after the fact! can do a follow-up PR based on responses

"main": {
"type": "record",
"key": "tid",
"description": "Record defining post URIs detached from a root post. The record key (rkey) of the detach record must match the record key of the root post in question, and that record must be in the same repository.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"detaching" works against both replies and quote-posts, right? either way we should probably clarify here in the description was "detatch" actually means.

"post": {
"type": "string",
"format": "at-uri",
"description": "Reference (AT-URI) to the post record."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Reference (AT-URI) to the original post record"

},
"description": "List of detached post URIs."
},
"updatedAt": { "type": "string", "format": "datetime" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this should get updated every time posts are added or removed from the targets array? should clarify in description.

},
"targets": {
"type": "array",
"maxLength": 50,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you said "1k" in the PR description, but only 50 here; was that intentional? either is probably ok/plausible to me, if I was painting the bike shed i'd probably go with 200

@estrattonbailey estrattonbailey mentioned this pull request Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants