Skip to content

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...

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 drozdowsky force-pushed the master branch 2 times, most recently from 44d7f41 to 71181e6 Compare January 27, 2024 18:00
@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
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.

3 participants