Metaprogramming proposed changed#83
Conversation
…forms checks on DB table defining class attributes of the Saveable classes
ScottSoren
left a comment
ScottSoren
left a comment
There was a problem hiding this comment.
Hi Kenneth,
This is an absolutely brilliant PR.
The DBML generation will make it so nice to polish all the table definition stuff, and develop db backends.
I've made comments here. Many of the comments deal with places on the dbdiagram that don't look as intended, in which case I've included a snip of the diagram. There's also a few questions and one important implementation suggestion.
To summarize the important points:
- There's a bit more to do for the linker tables. Not only primary table classes can have OwnedObjectLists and thus a new relationship requiring a linker table - inheriting classes that carry over the parent table id can also have these. The example here is
Field, which lives in the data_series table but needs a new relationship called axes_series. - Multiple inheritance is now handled well with respect to columns. For example,
ECMSCalibration.full_colums()now includes all the colums defined either inECCalibrationor inMSCalibration, as it should. (And the duplicate of theECColumnsthat I included before should now be deleted). However, multiple inheritance is a bit funky with respect to id, - I don't think including references to the id's from both base clases is best, since they should be the same anyway. I think it might be best for the id attribute to always refer as high up the class/table heigherarchy as possible, such that ecms_calibration only has calibration_id. - I had to read up on metaclasses, but now that I understand, I think this is a great use of it, and can do more! The "parsing" of the class structure into primary_table_classes and linker_tables now done in db_schema.py could be done with the metaclass. I think db backends would find that just as necessary as the DBML generator.
Sorry this PR has been sitting and waiting the whole summer. It deserves better :P .
| """ | ||
| new_cls = super().__new__(cls, name, bases, attrs) | ||
|
|
||
| if getattr(new_cls, "table_name") and "_SKIP_IN_TABLE_SCAN" not in attrs: |
There was a problem hiding this comment.
The _SKIP_IN_TABLE_SCAN attribute is not presently used anywhere. Do you expect it will be?
Also, I think we should add a comment that getattr(new_class, "table_name") will get the table name whether its defined in a base class or the new class, whereas the test for "table_name" in attrs below is specific to a new class.
I'm wondering if the metaclass's __new__() method should do some of the work presently done in generate_db_schema.py by storing the class in a different set based on whether it has a new table_name, and also storing the information on the linker tables.
There was a problem hiding this comment.
Err. I think the _SKIP_IN_TABLE_SCAN was something I added while I was developing the tool, that should just go away.
Agree on expanding with comments.
Agree that the metaclass could do more.
There was a problem hiding this comment.
Ah, we may need it anyway. See the comment in mother PR
| primary_table_classes = _extract_primary_classes( | ||
| table_to_class_mapping, verbose=verbose | ||
| ) | ||
| linker_tables = _extract_linker_tables(primary_table_classes, verbose=verbose) |
There was a problem hiding this comment.
Ah, but it's not just primary_table_classes that can have linker_tables. For example Field has a linker table called axes_series.
I'll think about how to deal with this.
There was a problem hiding this comment.
That table is actually a bit more complex even, because to be easily readable it should rename the columns so its clear which data series is the field and which data series is the axis, and also because the order of the axes matters it needs to include that.
Maybe the OwnedObjectList needs a few optional attributes to make this clear to the db schema:
| from ixdat.db import Saveable | ||
|
|
||
|
|
||
| def collect_db_classes(*, verbose=False): |
There was a problem hiding this comment.
I think that the work done here, as well as in _extract_primary_classes and _extract_linker_tables would all be better done in db.SaveableMetaclass.__new__()... I'll give that a try.
| # Sort the primary tables by their distance to Saveable (in terms of inheritance), | ||
| # and then by name, to make sure that tables that are higher on the inheritance | ||
| # chain are generated first | ||
| sorted_primary_table_classes = sorted( |
There was a problem hiding this comment.
I think that "tables that are higher on the inheritance chain are generated first" is also satisfied if the ordering of the classes is the order that SaveableMetaclass.__new__ adds them to a list, because an inheriting class can't be defined before its bas(es) are defined.
There was a problem hiding this comment.
But, but, I was so proud of the clever code :(
| if cls.parent_table_class: | ||
| return cls.parent_table_class.full_columns() + cls.columns | ||
| if isinstance(cls.parent_table_class, tuple): | ||
| assert len(cls.parent_table_class) == 2 |
There was a problem hiding this comment.
I don't understand the readon for this assertion.
Why can't the number of parent classes exceed 2?
(Not that I think it will, but I don't see from a programmatic perspective why it should be disallowed.)
There was a problem hiding this comment.
I think, if we ever add a triple combined technique we should change the check, but for now assume that if someone adds a tuple more than 2 somethings in it, that it is an error.
There was a problem hiding this comment.
Here it's a question of whether
- A thing should be disallowed unless its known to cause no problems and there's a known reason to do it.
- A thing should be allowed unless it's known to cause problems.
I'm solidly in camp 2.
| f"{id_column_name}]\n" | ||
| ) | ||
| else: # Single super class | ||
| id_column_name = f"{cls.parent_table_class.table_name.rstrip('s')}_id" |
| table_name_to_id_column_name[cls.table_name] = "id" | ||
|
|
||
| for column in cls.columns: | ||
| schema += f" {column.name} {_type_translation[column.ctype]}" |
| new_cls = super().__new__(cls, name, bases, attrs) | ||
|
|
||
| if getattr(new_cls, "table_name") and "_SKIP_IN_TABLE_SCAN" not in attrs: | ||
| ALL_SAVABLE_CLASSES.add(new_cls) |
There was a problem hiding this comment.
Why not generate PRIMARY_TABLE_CLASSES and LINKER_TABLES here in SaveableMetaClass.__new__ and save work later? (Elaborated in a later comment.)
|
Merging this as agreed, and continuing work on the [metaprogramming] branch and PR #75 . |
2 participants
This PR consist a set of proposed changes to the metaprogramming branch, that fell out of review and experimentation. The change are described in #75.