-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor anonymisable
interface into fields
and destroy
groups
#32
Conversation
6c48bcf
to
114f5d1
Compare
strategy | ||
end | ||
self.class.anonymise_config.validate! | ||
self.class.anonymise_config.apply(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm incredibly happy about this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And so you should be.
114f5d1
to
1fc73f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like it very much. Agree that moving over to this is a slight pain, but should be pretty mechanical.
We don't have to tackle it now, but I'm wondering if there's a use case for specifying your own "model-level" strategies?
strategy | ||
end | ||
self.class.anonymise_config.validate! | ||
self.class.anonymise_config.apply(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And so you should be.
7375f77
to
59fa47e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor comments, otherwise this looks good 👍
In preparation for an upcoming refactor, the Strategies namespace is going to be reused for a higher-level concept. This moves strategies relating to fields into a special module, FieldLevelStrategies. This commit also moves all of the strategies and their specs into a single file. This is because the definition of strategies should be effectively atomic with the loading of the dynamic registration code. This means that consumers which require this module will automatically get all of the necessary strategies configured for them without having to load separate files.
59fa47e
to
cf477fb
Compare
Since it's incompatible to have both
destroy
and field-level configuration, this PR modifies a fairly hefty chunk of the anony internals to accomplish what felt like a reasonable goal: isolating the field-level strategies and the destroy strategies into different classes. The code is structured so that it's impossible to define both of these, because the configuration is essentially an enum on a single instance variable (Anony::ModelConfig#@strategy
).This makes the public interface a bit more unwieldy, but I'd argue this makes sense in the context of some proposed changes in discussion. An example will hopefully be illustrative:
As you can see,
hex
andignore
don't logically belong at the same level asskip_if
orbefore
/after
. This isolates them into afields
block, which is encapsulated as anAnony::Strategies::Fields
instance (hence the first commit moving other things around). Sandi Metz would be proud of the subsequent approach: we delegate thevalid?
,validate!
andapply
methods to those strategies.