Skip to content

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.

3 participants