search

RegioHelden / django-scrubber

Anonymizer for Django database data
Other
25 stars 10 forks source link

Added hints to docs about overridden queryset method in manager #21

Closed GitRon closed 4 years ago

GitRon commented 4 years ago

Hi @costela

after our discussion the other day I thought if we don't solve the issue, we could at least inform people about it.

Ok for you?

Fixes https://github.com/RegioHelden/django-scrubber/issues/15

Best
Ronny

costela commented 4 years ago

Hi @GitRon ,

Thanks for the PR!

I'm a bit conflicted about this change, for similar reasons as outlined in https://github.com/RegioHelden/django-scrubber/pull/17#issuecomment-557889737. We'd be basically adding a warning to the README about usage which is IMHO pretty clearly an anti-pattern.
I think this is a bit of a slippery slope.

Furthermore, I don't think this a limitation of django-scrubber: it's a design decision.

If it turns out I'm overlooking something and there are valid use-cases for this "invisible rows" pattern, than we can either revisit your suggestion of always using the base manager, or turn this PR into something like a new FAQ.
Anyway I'll let this sit a while to see if someone else has any opinion on it.

GitRon commented 4 years ago

Hi @costela

ok, I agree. I too think its a bad way to code. But maybe we can find a different work than limition so people are warned?

Best Ronny

GitRon commented 4 years ago

@costela I changed the title. Agreed?

lociii commented 4 years ago

@GitRon thanks for your proposal but I will reject the pull request as this is not an issue specific to this library. Every other library relying on objects will be broken / misbehaving. Even the Django admin will not function as expected.