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

Fix 6 multi table inheritance #36

Closed
wants to merge 10 commits into from

Conversation

pierreben
Copy link

Hi,

In response to this ticket: #6:

I've created a fix to improve AggregateEvent query to also retrieve the parents values and their related models in case of multiple and complex inheritance.

I've added a test to validate the changes, and I've run the formater and the linter.

Can you give me your feedback on these updates ?

Many thanks !

@wesleykendall wesleykendall force-pushed the fix-6-multi-table-inheritance branch 2 times, most recently from 3eb4eaa to 42dd452 Compare July 31, 2022 01:33
@wesleykendall
Copy link
Member

@pierreben thanks for the change. I'm having a hard time figuring out what is different though because there are very many style changes. Even all single quotes are changed to double quotes.

I think you might have ran black with a different configuration.

I'll see if I can revert some of these changes myself

@wesleykendall
Copy link
Member

@pierreben I removed the commit that did black formatting, and that seemed to clear up the diff quite a bit.

I'm not totally sure I follow how this works with aggregate event tracking, but give me some more time to play around with it and understand.

If I understand correctly, this is merging together the parents with the children in the aggregate events? I wasn't sure what was going on in that code

@pierreben
Copy link
Author

Hi @wesleykendall ,

Thank you for all the time you spent on reformatting the code, sorry that my editor applies black everywhere.

This is quite a complex subject, so I think the best to help is that I put comments on the changes directly to illustrate them.

Don't hesitate to tell me if after these comments you need more details !

@pierreben
Copy link
Author

I saw that some of the changes I made weren't in last commits, I've retrieved the missing lines :)

@pierreben
Copy link
Author

@wesleykendall , I've tried to explain my updates as best as I could (sorry for my poor english).
I hope that it can help to understand how it works, don't hesitate to tell me if you need more details or explanations !

@pierreben
Copy link
Author

Hello @wesleykendall , I allow myself to follow up about this issue.
Don't hesitate to ask me for more details :)

@pierreben pierreben force-pushed the fix-6-multi-table-inheritance branch from a5e6dc5 to 6e0239f Compare October 19, 2022 16:04
@wesleykendall
Copy link
Member

Apologies for letting this PR get too stale. There have been many changes to pghistory since this and the original issue opened at #6

Although I don't think we have "full support" for concrete inheritance, there are ways to work around it for now mentioned at the end of that issue.

If people have other thoughts on how to better support concrete inheritance, please open a discussion and perhaps we can revisit this original PR if it still might solve inadequacies in the current version of pghistory

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.

2 participants