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 orga #494

Merged
merged 4 commits into from
Dec 22, 2023
Merged

automod orga #494

merged 4 commits into from
Dec 22, 2023

Conversation

warpfork
Copy link
Contributor

Several more package extractions.

These are all the easy ones -- flagstore, cachestore, setstore, directory -- all get yanked out in the same way as countstore already was.

These didn't cause any major changes to package graph overall, it just makes the main automod package smaller (yay). Near future targets are to try to tug engines and events a bit further apart, and that might be more complicated, so I thought I'd put up the easy-to-review diffs first.

Did not rename, so some names are stuttery (flagstore.FlagStore).
This can be fixed in a future pass if desired; today's goal is just
more packages and getting the filesystem tree approximately right.
Did not rename, so some names are stuttery (cachestore.CacheStore).
This can be fixed in a future pass if desired; today's goal is just
more packages and getting the filesystem tree approximately right.
Did not rename, so some names are stuttery (cachestore.CacheStore).
This can be fixed in a future pass if desired; today's goal is just
more packages and getting the filesystem tree approximately right.

I wanna log that I feel a little funky on this one.  I'm not fully
convinced that this actually needs an interface.  The main thing
that's expected to potentially change on this one is how it gets
loaded... but those methods _aren't in the interface anyway_.
The feature itself is providing data that's expected to be at low
(read: in-memory) latency: even if it was backed by redis as a data
source, the "load" function would be proactively forking that data
over into local memory.  A method on a concrete struct (so we're
still at least hiding direct access to the map) seems like it would
be sufficient.

But, all that said: maybe I'm wrong; there's very little harm in
having just one interface too many; and this is consistent with
the pattern of all the other things called "foostore".  So, fine.
This one will probably move further in the future -- there are already
directory service mocks elsewhere, for example, and so this should most
likely live near those.  That may deserve a larger code review,
however, so for now we just move it into a package as we do with other
things today.
@warpfork warpfork requested a review from bnewbold December 22, 2023 16:15
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.

nice!

@bnewbold bnewbold merged commit f886793 into main Dec 22, 2023
7 checks passed
@bnewbold bnewbold deleted the warpfork/automod-orga branch December 22, 2023 16:29
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