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

beemo: refactor and add mention notifications #674

Merged
merged 6 commits into from
Jun 27, 2024

Conversation

bnewbold
Copy link
Collaborator

No description provided.

@bnewbold bnewbold requested a review from ericvolp12 June 5, 2024 16:38
@bnewbold
Copy link
Collaborator Author

bnewbold commented Jun 5, 2024

@ericvolp12 nothing very high-stakes here but a skim and plus1 would be good if you get a sec

// NOTE: could add other callbacks as needed
}

var scheduler events.Scheduler
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend using the parallelScheduler tbh, the autoscalingScheduler has been more trouble than it's worth lately.

if mention == nil {
continue
}
for _, d := range mc.mentionDIDs {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a for loop over a list here, you could make mentionDIDs a map and key into it, would be a bit more idiomatic Go if that's the only way we're using that struct (to check membership), turn mentionDIDs into a map[string]struct{} which is Go's version of a "set"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are correct, but i'm going to be lazy and skip on this refactor in this case. i'll try to use that pattern more in the future

// query just new reports (regardless of resolution state)
// ModerationQueryEvents(ctx context.Context, c *xrpc.Client, createdBy string, cursor string, includeAllUserRecords bool, limit int64, sortDirection string, subject string, types []string) (*ModerationQueryEvents_Output, error)
var limit int64 = 50
me, err := toolsozone.ModerationQueryEvents(
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol lmao, love this method

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, it's a bummer what is fairly reasonable/idiomatic for API design ends up so bad in codegen here. several ways we could re-architect the codegen some day. maybe worth creating a more ergonomic wrapper (with config struct) for a handful of these monster methods.

Copy link
Collaborator

@ericvolp12 ericvolp12 left a comment

Choose a reason for hiding this comment

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

LGTM aside from a few nits, no need to fix them tho if you don't mind

@bnewbold bnewbold merged commit d4ddb54 into main Jun 27, 2024
7 checks passed
@bnewbold bnewbold deleted the bnewbold/beemo-iteration branch June 27, 2024 19:25
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.

2 participants