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

Add an Eve-aware base class (WIP) #180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nicolaiarocci
Copy link
Member

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.

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.
@nicolaiarocci nicolaiarocci requested a review from dkellner August 7, 2018 09:02
@nicolaiarocci nicolaiarocci force-pushed the add_eve_aware_base_class branch from 4bd4c18 to 8094137 Compare August 7, 2018 09:48
@nicolaiarocci nicolaiarocci force-pushed the add_eve_aware_base_class branch from 8094137 to 21d43c0 Compare August 7, 2018 10:02
Copy link
Collaborator

@dkellner dkellner left a comment

Choose a reason for hiding this comment

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

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 from eve_sqlalchemy would be nice.
  • For me, DefaultBaseModel fits better as it conveys it's not applicable for any use case.
  • Now that the docs will not include the definition of CommonColumns anymore, 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.

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