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.

Sign up for GitHub

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

Jump to bottom

Add ability to prevent anonymisation #25

Merged
merged 2 commits into from
Dec 4, 2019
Merged

Add ability to prevent anonymisation #25

merged 2 commits into from
Dec 4, 2019

Conversation

nickcampbell18
Copy link
Contributor

In some circumstances, we don't want to anonymise a record (for example, imagine a record which is shared amongst multiple users). This PR introduces that interface.

nickcampbell18
nickcampbell18 commented Nov 25, 2019
View reviewed changes
README.md Outdated Show resolved Hide resolved
danwakefield
danwakefield reviewed Nov 25, 2019
View reviewed changes
README.md Outdated Show resolved Hide resolved
lib/anony/anonymisable.rb Outdated Show resolved Hide resolved
spec/anony/anonymisable_spec.rb Outdated Show resolved Hide resolved
@nickcampbell18 nickcampbell18 force-pushed the never-say-never branch 2 times, most recently from 3f82952 to 0f1a1cf Compare November 25, 2019 21:32
@nickcampbell18

This comment has been minimized.

Sign in to view
@matt-thomson
Copy link
Contributor

I do like the idea of having fields - now that we have two exceptions I think it'd make sense to do that. Would be a breaking change but should be fairly mechanical to fix, so I say we bite the bullet.

I'm not 100% sold on the name never though - it's quite a strong word, so feels odd for it then to be conditional. I realise that naming things is hard, though, so I could go with it if we can't think of a better alternative...

matt-thomson
matt-thomson reviewed Nov 26, 2019
View reviewed changes
Copy link
Contributor

@matt-thomson matt-thomson left a comment

Choose a reason for hiding this comment

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

I think I'm going slightly cold on adding so many methods to the model when we include Anonymisable. Don't have to fix on this PR but I reckon we might want to refactor this a bit...

lib/anony/anonymisable.rb Outdated Show resolved Hide resolved
lib/anony/anonymisable.rb Outdated Show resolved Hide resolved
@stephenbinns
Copy link
Contributor

I'm not 100% sold on the name never though

Agree with this and propose skip

stephenbinns
stephenbinns approved these changes Dec 4, 2019
View reviewed changes
Copy link
Contributor

@stephenbinns stephenbinns left a comment

Choose a reason for hiding this comment

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

💯

@nickcampbell18
Copy link
Contributor Author

Ah sorry @stephenbinns I switched from :skipped to a raise - thoughts?

matt-thomson
matt-thomson reviewed Dec 4, 2019
View reviewed changes
lib/anony/model_config.rb
# @example
# Anony::ModelConfig.new(Manager).apply(Manager.new)
def apply(instance)
raise Anony::SkippedException if @skip_filter && instance.instance_exec(&@skip_filter)
Copy link
Contributor

Choose a reason for hiding this comment

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

I felt slightly funny about the mismatch between throwing an exception and the name skip. On balance I think this is OK: the exception is the behaviour we want, and I can't think of a better name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until we reach 1.0 we can move these around as needed.

@nickcampbell18 nickcampbell18 merged commit 6cb8178 into master Dec 4, 2019
@nickcampbell18 nickcampbell18 deleted the never-say-never branch December 4, 2019 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matt-thomson matt-thomson matt-thomson approved these changes

@stephenbinns stephenbinns stephenbinns approved these changes

@matthieuprat matthieuprat Awaiting requested review from matthieuprat

@xmjw xmjw Awaiting requested review from xmjw

@danwakefield danwakefield Awaiting requested review from danwakefield

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nickcampbell18 @matt-thomson @stephenbinns @danwakefield