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

cmd/bsky-webhook: add support for facets #11

Merged
merged 2 commits into from
Mar 20, 2025
Merged

cmd/bsky-webhook: add support for facets #11

merged 2 commits into from
Mar 20, 2025

Conversation

Erisa
Copy link
Member

@Erisa Erisa commented Jan 2, 2025

Reference: https://docs.bsky.app/docs/advanced-guides/post-richtext
Fixes #9

Tested working with links, mentions, and tags. Seeking a quick review to ensure I didn't commit any Go crimes in the process.

@Erisa Erisa requested a review from creachadair January 2, 2025 23:25
type BskyFacetFeatures struct {
Type string `json:"$type"`
Uri string `json:"uri"`
Did string `json:"did"`
Copy link
Member

@creachadair creachadair Jan 3, 2025

Choose a reason for hiding this comment

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

Do we need the names explicitly here? It appears we only use these for decoding, and the JSON decoder does a case-insensitive match anyway. (We do need $type because of the spelling difference, but I mean the others)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think (personally) that being explicit is best because it doesn't result in confusion in naming or capitalisation later down the line if we do encode back again, especially since some other types SlackAttachment and SlackBody are used for encoding.

I'm willing to move on that if a Go convention says that they shouldn't be explicit, though!

return a.Index.ByteStart - b.Index.ByteStart
})

textCursor := 0
Copy link
Member

@creachadair creachadair Jan 3, 2025

Choose a reason for hiding this comment

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

If I understand correctly, the delicate dance with indices here is something to do with the fact that facets are specified by offsets into the base text?

Given that you went to the trouble of ordering them above, maybe this could be turned into a helper that uses slices.BinarySearch (or even just scans them linearly—there aren't going to be that many, we don't need to worry about asymptotics I think) to find the relevant fragment for each facet? (Even if we do care, we already spent O(n lg n) to sort them, so spending another n lg n to probe them won't affect the underlying shape)

I am sort of convinced this does what I described above, but index dancing is hard to follow, so I would suggest we try to be more obvious here.

(Then too, I think maybe we don't need to do any additional appending, since the facet decorations could be glued directly into place on the facet fragment they belong to I think?)

@Erisa Erisa requested a review from creachadair March 17, 2025 17:28
@Erisa Erisa merged commit bc18f7d into main Mar 20, 2025
1 check passed
@Erisa Erisa deleted the erisa/facets branch March 20, 2025 10:41
@tailscale tailscale deleted a comment from creachadair Mar 20, 2025
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.

Links are being truncated
2 participants