Open
Conversation
The idea is to provide a base class from which user Tables can inherit. Unless they are meant for read-only acces, tables will have to provide these fields anyway. Not sure on a few things: - namespace should be different? Maybe have the base-class reside in the main eve_sqlalchemy namespace, or something different than 'declarative' to avoid confusion with sqlalchemy itself (I actually followed that lead). - base class name. Maybe CommonColumns, or EveBaseModel, or something else would be more appropriate? - What if the user changes ETAG, CREATED, UPDATED settings in the main Eve app? Again, I am not confident enough with the extension code, so careful review would be appreciated.
4bd4c18 to
8094137
Compare
8094137 to
21d43c0
Compare
dkellner
requested changes
Nov 10, 2018
Collaborator
dkellner
left a comment
There was a problem hiding this comment.
You're right, most users will want to use something like this anyway, so we should provide it. But the drawbacks with changed settings are valid, too.
- I'd leave it in
declarative.py, but import fromeve_sqlalchemywould be nice. - For me,
DefaultBaseModelfits better as it conveys it's not applicable for any use case. - Now that the docs will not include the definition of
CommonColumnsanymore, they should mention all columns the new base class provides, including the warning that changing the above mentioned settings will make an own base class necessary.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The idea is to provide a base class from which user Tables can inherit.
Unless they are meant for read-only acces, tables will have to provide
these fields anyway.
Not sure on a few things:
main eve_sqlalchemy namespace, or something different than 'declarative'
to avoid confusion with sqlalchemy itself (I actually followed that
lead).
else would be more appropriate?
Eve app?
Again, I am not confident enough with the extension code, so careful
review would be appreciated.