Ticket #89 (closed enhancement: fixed)

Opened 3 years ago

Last modified 3 years ago

"abstract" entity option like Abstract base classes feature in Django ORM

Reported by: guest Owned by: ged
Priority: normal Milestone: 0.7.1
Component: core Version: 0.6.1
Keywords: abstract Cc:

Description

Hi,

this is my patch about this discussion : http://groups.google.com/group/sqlelixir/browse_thread/thread/8c84e8c86c36b93f#

Example of use in "tests/test_abstract.py" file.

TODO :

  • I need to write some documentation in Elixir wiki
  • I need to work on "test_simple_relation2" test

Regards, Stephane

Attachments

elixir.abstract.patch (6.0 kB) - added by guest 3 years ago.
Elixir abstract entity patch
elixir.abstract.2.patch (7.2 kB) - added by guest 3 years ago.
New patch version
elixir.abstract.3.patch (7.7 kB) - added by guest 3 years ago.
New patch with test_simple_relation2 pratical
elixir.patch (12.4 kB) - added by guest 3 years ago.
New version with multi inheritance feature
elixir.diff (9.0 kB) - added by guest 3 years ago.

Change History

Changed 3 years ago by guest

Elixir abstract entity patch

Changed 3 years ago by guest

New patch version

Changed 3 years ago by guest

New patch with test_simple_relation2 pratical

Changed 3 years ago by guest

New version with multi inheritance feature

follow-up: ↓ 4   Changed 3 years ago by ged

The multiple inheritance idea is cool. Your patch is getting along nicely.

There are still a few issue I have before I can apply the patch: - I much prefer tests which are only as long as strictly necessary. Many of your tests could be shortened. For example, even though the Enum code is interesting, I don't see why it should be included within the test suite.

- In the same vein, "assert 'firstname' in Employed.table.columns.keys()" can be written as: "assert 'firstname' in Employed.table". I know other tests used that idiom too. This is a left-over code to support earlier SA version which didn't implement contains on the ColumnCollection. This is not needed anymore (and I've just changed occurrences of that idiom in the other tests). - style does not respect PEP8 (http://www.python.org/dev/peps/pep-0008/). Your most frequent "error" is to put spaces around the = sign of keyword arguments as in "using_options(abstract = True)", which should be "using_options(abstract=True)"

  Changed 3 years ago by guest

About Enum test, I've append it because it don't work if copy is used in elixir/entity.py instead of deepcopy.

This test supervise this point.

  Changed 3 years ago by guest

About Enum test, I've append it because it don't work if copy is used in elixir/entity.py instead of deepcopy.

This test supervise this point.

Well, my comment is wrong.

I've append Enum test because Enum class need to have "_deepcopy_" method to work several Abstract Entity derivation.

in reply to: ↑ 1   Changed 3 years ago by guest

Replying to ged:

The multiple inheritance idea is cool. Your patch is getting along nicely. There are still a few issue I have before I can apply the patch: - I much prefer tests which are only as long as strictly necessary. Many of your tests could be shortened. For example, even though the Enum code is interesting, I don't see why it should be included within the test suite. - In the same vein, "assert 'firstname' in Employed.table.columns.keys()" can be written as: "assert 'firstname' in Employed.table". I know other tests used that idiom too. This is a left-over code to support earlier SA version which didn't implement contains on the ColumnCollection. This is not needed anymore (and I've just changed occurrences of that idiom in the other tests). - style does not respect PEP8 (http://www.python.org/dev/peps/pep-0008/). Your most frequent "error" is to put spaces around the = sign of keyword arguments as in "using_options(abstract = True)", which should be "using_options(abstract=True)"

I've fixed all this item in my new patch.

Regards, Stephane

Changed 3 years ago by guest

  Changed 3 years ago by ged

  • owner set to ged
  • status changed from new to accepted
  • milestone changed from 0.7 to 0.8

  Changed 3 years ago by ged

  • milestone changed from 0.8 to 0.7.1

  Changed 3 years ago by ged

  • status changed from accepted to closed
  • resolution set to fixed

commited in r511. Thanks a lot for the patch and sorry it took so long.

Note: See TracTickets for help on using tickets.