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

automod context plumbing #487

Closed
wants to merge 3 commits into from
Closed

Conversation

warpfork
Copy link
Contributor

Plumb context through rules.

This is mostly not yet used, but prepares for future needs. It does also remove one TODO -- there was one existing rule that uses a directory lookup, and was doing so without a fully connected context.

I took the liberty of adding type assertion "variables" to the header of each rule file, while at it.

(Currently also includes a few commits from another branch, but will rebase as soon as that's merged.)

A rule's name should describe what it does, not presumptively describe
the valence of what it hopes that it's detecting.
Context objects are now available in all rules.

This is mostly not yet used, but prepares for future needs.
It does also remove one TODO -- in a rule that uses a directory lookup.

I took the liberty of adding type assertion "variables" to the header
of each rule file, while at it.
@bnewbold
Copy link
Collaborator

bnewbold commented Dec 19, 2023

What do you think about plumbing the context.Context through the event itself, to avoid having an extra argument in the function signature? Eg, similar to net/http HandlerFunc not taking a ctx argument, but having access to it via Request.Context().

@warpfork
Copy link
Contributor Author

We can discard this. Even as I was making these edits, I was coming around to thinking... yeah, the sheer args list length feels high and noisy. I agree making some kind of "business context" one-stop-shop object feels like it will make a lot of sense. We might as well start that in a new PR.

@warpfork warpfork closed this Dec 22, 2023
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