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

Automatically Set CreatedAt and UpdatedAt #244

Merged
merged 13 commits into from
Sep 22, 2024

Conversation

TheTedder
Copy link
Contributor

@TheTedder TheTedder commented Sep 6, 2024

What it says on the tin. Had to mess with the tests a bit to get this working properly. Also removes erroneously overridden Equals and GetHashCode on models (this is a no-no). Closes #237

@zysim
Copy link
Collaborator

zysim commented Sep 7, 2024

Kinda don't like having inheritance on the interfaces themselves. If we do go down with the interfaces approach; I'd rather we do something like this: (actually it really doesn't matter. Though I would still prefer if we did the base class, though)

public class Leaderboard : ICreatedAt, IUpdatedAt

Alternatively as I've said in the channel; we just do a base class with all three timestamps, and have classes inherit that

@TheTedder
Copy link
Contributor Author

Kinda don't like having inheritance on the interfaces themselves. If we do go down with the interfaces approach; I'd rather we do something like this: (actually it really doesn't matter. Though I would still prefer if we did the base class, though)

public class Leaderboard : ICreatedAt, IUpdatedAt

Alternatively as I've said in the channel; we just do a base class with all three timestamps, and have classes inherit that

Having one base class with all three is impossible since some tables have CreatedAt but not UpdatedAt. I couldn't see an easy way to automate DeletedAt so I'm keeping that one manual for now. Would you still like to me get rid of the inheritance?

@zysim
Copy link
Collaborator

zysim commented Sep 21, 2024

Yeah nah just keep what you have. Don't have to that class inheritance thing.

@TheTedder TheTedder marked this pull request as ready for review September 21, 2024 04:29
@TheTedder TheTedder requested a review from a team as a code owner September 21, 2024 04:29
@zysim
Copy link
Collaborator

zysim commented Sep 21, 2024

How'd you figure out on how to add methods in ApplicationContext like that?

@TheTedder
Copy link
Contributor Author

How'd you figure out on how to add methods in ApplicationContext like that?

I'm actually a wizard IRL. Also I read this article.

@zysim
Copy link
Collaborator

zysim commented Sep 21, 2024

Would IHasUpdateTimestamp trigger on creation as well?

@TheTedder
Copy link
Contributor Author

Would IHasUpdateTimestamp trigger on creation as well?

I'm not sure but it doesn't really matter either way as far as I'm concerned.

@zysim zysim merged commit 4021aff into leaderboardsgg:main Sep 22, 2024
1 check passed
@TheTedder TheTedder deleted the auto-timestamps branch September 22, 2024 20:13
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.

Automatically Set UpdatedAt Fields
2 participants