tangled.org / core
Monorepo for Tangled tangled.org
fork atom

appview/notify: merge new comment events into one #867

open opened by boltless.me targeting master from sl/wnrvrwyvrlzo
STACK 4
#867 appview/notify: merge new comment events into one
1/3
6
#7
#866 appview: replace IssueComment to Comment
1/3
1
#7
#865 appview: replace PullComment to Comment
1/3
4
#7
#864 lexicons: add general sh.tangled.comment lexicon
1/3
0
#7
Labels

None yet.

assignee

None yet.

Participants 3
AT URI
at://did:plc:xasnlahkri4ewmbuzly2rlc5/sh.tangled.repo.pull/3m7iohv2yba22
Interdiff #1 → #2
unified split
appview/issues/issues.go

This file has not been changed.

appview/notify/db/db.go

This patch was likely rebased, as context lines do not match.

appview/notify/merged_notifier.go

This patch was likely rebased, as context lines do not match.

appview/notify/notifier.go

This file has not been changed.

appview/notify/posthog/notifier.go

This file has not been changed.

appview/pulls/pulls.go

This file has not been changed.

History

8 rounds 6 comments
sign up or login to add to the discussion
boltless.me submitted #7
diff interdiff
1 commit
expand
77b11ed9
appview/notify: merge new comment events into one
2/3 failed, 1/3 success
expand
build.yml
fmt.yml
test.yml
merge conflicts detected
expand
  • appview/notify/db/db.go:260
  • appview/notify/merged_notifier.go:81
  • appview/notify/notifier.go:22
  • appview/pulls/opengraph.go:277
expand 0 comments
boltless.me submitted #6
diff interdiff
1 commit
expand
a2a6cc1e
appview/notify: merge new comment events into one
1/3 failed, 2/3 success
expand
build.yml
fmt.yml
test.yml
expand 0 comments
boltless.me submitted #5
diff interdiff
1 commit
expand
89722dba
appview/notify: merge new comment events into one
3/3 success
expand
build.yml
fmt.yml
test.yml
expand 0 comments
boltless.me submitted #4
diff interdiff
1 commit
expand
92ce59a4
appview/notify: merge new comment events into one
3/3 success
expand
build.yml
fmt.yml
test.yml
expand 2 comments
oppi.li

on the whole this changeset lgtm. some questions i have regarding backwards compatibility:

  • it makes sense that records created with sh.tangled.issue.comment will not be ingested anymore
  • however, when we intend to perform backfill of records, we should support ingesting the old records. i think the code itself is quite an easy undertaking, but the value add will be huge (if we can backfill existing issue comments!), this need not be added into the present PR, it can come down the line (with your work on backfilling with tap perhaps)
  • we should also allow users to delete/update old records via firehose (so deletions/updates of sh.tangled.issue.comment should reflect correctly). as above, the ingester logic for this can come down the line.
  • we can stop maintaining the old record for much longer, although i suspect that the new comment record will be quite stable
boltless.me

@oppi.li thank you for the review!

when we intend to perform backfill of records, we should support ingesting the old records.

Yeah I think it would be better to leave ingester logic for issue.comment update/delete operations. Users should be able to modify existing records.

Backfilling issue.comment will be quite tricky as we cannot distinguish new comments since certain timestamp. Users can modify both created field and rkey. So there are only two options:

  • ingest legacy comment records until we completely drop them (we should silently migrate user records as much as possible before that)
  • have "legacy records" list in tangled appview and only ingest for those records. This way, other appviews won't be able to support legacy records.

we can stop maintaining the old record for much longer, although i suspect that the new comment record will be quite stable

There are few issues that can threat the comment lexicon stability:

I think we are roughly fine after changing body field type to sh.tangled.markup#markdown. Though we might we need more discussion for #328 just in case.

boltless.me submitted #3
diff interdiff
1 commit
expand
f3a98fd4
appview/notify: merge new comment events into one
1/3 failed, 2/3 success
expand
build.yml
fmt.yml
test.yml
expand 0 comments
boltless.me submitted #2
diff
1 commit
expand
d6eaecd5
appview/notify: merge new comment events into one
3/3 success
expand
build.yml
fmt.yml
test.yml
expand 0 comments
boltless.me submitted #1
diff interdiff
1 commit
expand
08c1258b
appview/notify: merge new comment events into one
3/3 success
expand
build.yml
fmt.yml
test.yml
expand 1 comment
boltless.me submitted #0
diff
1 commit
expand
1a41a366
appview/notify: merge new comment events into one
1/3 failed, 2/3 timeout
expand
build.yml
fmt.yml
test.yml
expand 3 comments
boltless.me

note: I'm choosing sh.tangled.comment NSID here. Tell me if we would prefer something else like sh.tangled.feed.comment

anirudh.fi

I think sh.tangled.comment is fine, but more importantly, are we still sticking to sh.tangled or does it make sense to move this to the org.tangled namespace? Assuming we're moving everything eventually. cc @oppi.li

boltless.me

I think we should stick on sh.tangled for now as there can be large amounts of changes from PR refactor / did-for-repo. We can consider moving entire collections to org.tangled once tangled lexicons become more stable.