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

Allow Users to override tracker class #4

Merged
merged 4 commits into from
Jan 29, 2024

Conversation

sumit4613
Copy link
Contributor

Motivation

Currently Tracker is tightly coupled with TrackingModelMixin, if we want to add some methods that replicate the behavior of model_utils.FieldTracker methods, we need to override the TrackingModelMixin and rewrite the tracker property and write a new Tracker class with the required methods.

To avoid this, I'm adding a new attribute tracker_class to TrackingModelMixin which defaults to Tracker.

This helps us to easily override the Tracker and write required methods, without duplicated lines of code.

@drozdowsky @browniebroke please let me know what you think about this approach.

Finally, thank you ❤️ for creating this library, it helped me solve some problems that were coming up because of FieldTracker.

Copy link
Contributor

@browniebroke browniebroke left a comment

Choose a reason for hiding this comment

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

I'm not a maintainer, but as a user of this lib, I can see how this might be useful 👍🏻

I left an idea/suggestion, although it doesn't have to be implemented.

tests/models.py Outdated
Comment on lines 42 to 43
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this doing anything? Seems like it can be removed...

@@ -12,11 +12,24 @@ def __init__(self, instance):
class TrackingModelMixin(object):

TRACKED_FIELDS = None
tracker_class = Tracker
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine it might convenient to be able to configure the same Tracker class project-wide, rather than setting this attribute on each models that subclass TrackingModelMixin.

With that in mind, I would suggest a Django setting TRACKING_MODEL_TRACKER_CLASS (or something along these lines) to set an import path for a class to use everywhere, whihc value would default to "tracking_model.Tracker".

However, it does make the implementation a bit more complicated. A keyword here is "imagine": it's not a problem I faced, just something I think I might need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

able to configure the same Tracker class project-wide

For project-wide usage, I thought that one could easily subclass TrackingModelMixin and override the tracking_class and use it everywhere. (Took DRF's approach here)

But yeah, from the end user perspective, this settings approach would completely eliminate the overriding.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's actually a great solution, much simpler and direct than a setting.

@drozdowsky
Copy link
Owner

Hey, thanks for PR, I will look into it shortly, seems like I don't get notifications here so sorry for late response (gotta make sure it does not happen again too).
I still have to think it through if we can make it a bit more simple but I like the overall approach and seems like a good idea

@drozdowsky
Copy link
Owner

Hi @sumit4613,
Please see my 'improvements', if you have no objections I will merge it, let me know, once again thanks for contrib.!

@sumit4613
Copy link
Contributor Author

Hey @drozdowsky, your changes look good to me. Please go ahead, no objections.

@drozdowsky drozdowsky merged commit e1cd44f into drozdowsky:master Jan 29, 2024
2 checks passed
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.

None yet

3 participants