From a9d5b4d5af4bbd2820709dda5600c110a6663618 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Fri, 16 Jul 2021 20:15:04 +0200 Subject: [PATCH] (core) Refactor Table.Record[Set] classes Summary: Dealing with some things that bothered and sometimes confused me: Make Table.Record[Set] provide the table argument automatically Remove the classes from UserTable because they're not used anywhere and the Table/UserTable distinction is already confusing. They're not documented for users and they don't show up in autocomplete. Remove RecordSet.Record because it was confusing me where that attribute was being set, but also this means .Record will work properly for users with columns named 'Record'. Test Plan: existing tests Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2913 --- sandbox/grist/autocomplete_context.py | 2 ++ sandbox/grist/column.py | 4 ++-- sandbox/grist/docmodel.py | 2 +- sandbox/grist/engine.py | 2 +- sandbox/grist/records.py | 22 ++++++++++++------- sandbox/grist/table.py | 31 ++++++++++++++++++++------- sandbox/grist/test_completion.py | 2 ++ sandbox/grist/test_gencode.py | 3 --- sandbox/grist/test_lookups.py | 4 ++-- sandbox/grist/test_renames.py | 2 +- 10 files changed, 48 insertions(+), 26 deletions(-) diff --git a/sandbox/grist/autocomplete_context.py b/sandbox/grist/autocomplete_context.py index 1f28614a..765fc5b8 100644 --- a/sandbox/grist/autocomplete_context.py +++ b/sandbox/grist/autocomplete_context.py @@ -42,6 +42,8 @@ class AutocompleteContext(object): # Add in the important UserTable methods, with custom friendlier descriptions. self._functions['.lookupOne'] = Completion('.lookupOne', '(colName=, ...)', True) self._functions['.lookupRecords'] = Completion('.lookupRecords', '(colName=, ...)', True) + self._functions['.Record'] = Completion('.Record', '', True) + self._functions['.RecordSet'] = Completion('.RecordSet', '', True) # Remember the original name for each lowercase one. self._lowercase = {} diff --git a/sandbox/grist/column.py b/sandbox/grist/column.py index 52aa77e8..4d956084 100644 --- a/sandbox/grist/column.py +++ b/sandbox/grist/column.py @@ -421,7 +421,7 @@ class ReferenceColumn(BaseReferenceColumn): return typed_value # For a Reference, values must either refer to an existing record, or be 0. In all tables, # the 0 index will contain the all-defaults record. - return self._target_table.Record(self._target_table, typed_value, self._relation) + return self._target_table.Record(typed_value, self._relation) def _update_references(self, row_id, old_value, new_value): if old_value: @@ -452,7 +452,7 @@ class ReferenceListColumn(BaseReferenceColumn): # If we refer to an invalid table, return integers rather than fail completely. if not self._target_table: return typed_value - return self._target_table.RecordSet(self._target_table, typed_value, self._relation) + return self._target_table.RecordSet(typed_value, self._relation) def _raw_get_without(self, row_id, target_row_ids): """ diff --git a/sandbox/grist/docmodel.py b/sandbox/grist/docmodel.py index c6c12379..22b747a1 100644 --- a/sandbox/grist/docmodel.py +++ b/sandbox/grist/docmodel.py @@ -253,7 +253,7 @@ class DocModel(object): table_obj = record_set_or_table.table row_ids = self._engine.user_actions.BulkAddRecord(table_obj.table_id, [None] * count, values) - return [table_obj.Record(table_obj, r, None) for r in row_ids] + return [table_obj.Record(r, None) for r in row_ids] def insert(self, record_set, position, **col_values): """ diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index f7672f9f..5e67302e 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -860,7 +860,7 @@ class Engine(object): usercode_reference = self.gencode.usercode checkpoint = self._get_undo_checkpoint() - record = table.Record(table, row_id, table._identity_relation) + record = table.Record(row_id, table._identity_relation) try: if cycle: raise depend.CircularRefError("Circular Reference") diff --git a/sandbox/grist/records.py b/sandbox/grist/records.py index 8546622a..c6af60c5 100644 --- a/sandbox/grist/records.py +++ b/sandbox/grist/records.py @@ -61,6 +61,12 @@ class Record(object): table - Table object, in which this record lives. row_id - The ID of the record within table. relation - Relation object for how this record was obtained; used in dependency tracking. + + In general you shouldn't call this constructor directly, but rather: + + table.Record(row_id, relation) + + which provides the table argument automatically. """ self._table = table self._row_id = row_id @@ -103,8 +109,8 @@ class Record(object): return "%s[%s]" % (self._table.table_id, self._row_id) def _clone_with_relation(self, src_relation): - return self.__class__(self._table, self._row_id, - relation=src_relation.compose(self._source_relation)) + return self._table.Record(self._row_id, + relation=src_relation.compose(self._source_relation)) class RecordSet(object): @@ -157,7 +163,7 @@ class RecordSet(object): def __iter__(self): for row_id in self._row_ids: - yield self.Record(self._table, row_id, self._source_relation) + yield self._table.Record(row_id, self._source_relation) def __contains__(self, item): """item may be a Record or its row_id.""" @@ -169,7 +175,7 @@ class RecordSet(object): def get_one(self): row_id = min(self._row_ids) if self._row_ids else 0 - return self.Record(self._table, row_id, self._source_relation) + return self._table.Record(row_id, self._source_relation) def _get_col(self, col_id): return self._table._get_col_subset(col_id, self._row_ids, self._source_relation) @@ -180,10 +186,10 @@ class RecordSet(object): return self._table._attribute_error(name, self._source_relation) def _clone_with_relation(self, src_relation): - return self.__class__(self._table, self._row_ids, - relation=src_relation.compose(self._source_relation), - group_by=self._group_by, - sort_by=self._sort_by) + return self._table.RecordSet(self._row_ids, + relation=src_relation.compose(self._source_relation), + group_by=self._group_by, + sort_by=self._sort_by) class ColumnView(object): diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index 2e42636e..9f4a863d 100644 --- a/sandbox/grist/table.py +++ b/sandbox/grist/table.py @@ -61,14 +61,19 @@ class UserTable(object): self.Model = model_class column_ids = {col for col in model_class.__dict__ if not col.startswith("_")} column_ids.add('id') - self.Record = type('Record', (records.Record,), {}) - self.RecordSet = type('RecordSet', (records.RecordSet,), {}) - self.RecordSet.Record = self.Record self.table = None def _set_table_impl(self, table_impl): self.table = table_impl + @property + def Record(self): + return self.table.Record + + @property + def RecordSet(self): + return self.table.RecordSet + # Note these methods are named camelCase since they are a public interface exposed to formulas, # and we decided camelCase was a more user-friendly choice for user-facing functions. def lookupRecords(self, **field_value_pairs): @@ -200,6 +205,18 @@ class Table(object): # For a summary table, the name of the special helper column auto-added to the source table. self._summary_helper_col_id = None + # Add Record and RecordSet subclasses which fill in this table as the first argument + class Record(records.Record): + def __init__(inner_self, *args, **kwargs): # pylint: disable=no-self-argument + super(Record, inner_self).__init__(self, *args, **kwargs) + + class RecordSet(records.RecordSet): + def __init__(inner_self, *args, **kwargs): # pylint: disable=no-self-argument + super(RecordSet, inner_self).__init__(self, *args, **kwargs) + + self.Record = Record + self.RecordSet = RecordSet + def _rebuild_model(self, user_table): """ Sets class-wide properties from a new Model class for the table (inner class within the table @@ -207,8 +224,6 @@ class Table(object): """ self.user_table = user_table self.Model = user_table.Model - self.Record = user_table.Record - self.RecordSet = user_table.RecordSet new_cols = collections.OrderedDict() new_cols['id'] = self._id_column @@ -337,7 +352,7 @@ class Table(object): row_ids = sorted(row_id_set, key=lambda r: self._get_col_value(sort_by, r, rel)) else: row_ids = sorted(row_id_set) - return self.RecordSet(self, row_ids, rel, group_by=kwargs, sort_by=sort_by) + return self.RecordSet(row_ids, rel, group_by=kwargs, sort_by=sort_by) def lookup_one_record(self, **kwargs): return self.lookup_records(**kwargs).get_one() @@ -416,7 +431,7 @@ class Table(object): # user-actions caused by formula side-effects (e.g. as trigged by lookupOrAddDerived()) if row_id not in self.row_ids: raise KeyError("'get_record' found no matching record") - return self.Record(self, row_id, None) + return self.Record(row_id, None) def filter_records(self, **kwargs): """ @@ -427,7 +442,7 @@ class Table(object): # See note in get_record() about using this call from formulas. for row_id in self.filter(**kwargs): - yield self.Record(self, row_id, None) + yield self.Record(row_id, None) # TODO: document everything here. diff --git a/sandbox/grist/test_completion.py b/sandbox/grist/test_completion.py index 0fcc4241..03ab23e8 100644 --- a/sandbox/grist/test_completion.py +++ b/sandbox/grist/test_completion.py @@ -213,6 +213,8 @@ class TestCompletion(test_engine.EngineTestCase): def test_suggest_lookup_methods(self): # Should suggest lookup formulas for tables. self.assertEqual(self.engine.autocomplete("Address.", "Students", "firstName", self.user), [ + ('Address.Record', '', True), + ('Address.RecordSet', '', True), 'Address.all', ('Address.lookupOne', '(colName=, ...)', True), ('Address.lookupRecords', '(colName=, ...)', True), diff --git a/sandbox/grist/test_gencode.py b/sandbox/grist/test_gencode.py index 07fa9894..c73b76f1 100644 --- a/sandbox/grist/test_gencode.py +++ b/sandbox/grist/test_gencode.py @@ -82,9 +82,6 @@ class TestGenCode(unittest.TestCase): gcode.make_module(self.schema) module = gcode.usercode self.assertTrue(isinstance(module.Students, table.UserTable)) - self.assertTrue(issubclass(module.Students.Record, records.Record)) - self.assertTrue(issubclass(module.Students.RecordSet, records.RecordSet)) - self.assertIs(module.Students.RecordSet.Record, module.Students.Record) def test_pick_col_ident(self): self.assertEqual(identifiers.pick_col_ident("asdf"), "asdf") diff --git a/sandbox/grist/test_lookups.py b/sandbox/grist/test_lookups.py index ebcc2115..928387a8 100644 --- a/sandbox/grist/test_lookups.py +++ b/sandbox/grist/test_lookups.py @@ -633,7 +633,7 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName # A helper for comparing Record objects below. schools_table = self.engine.tables['Schools'] def SchoolsRec(row_id): - return schools_table.Record(schools_table, row_id, None) + return schools_table.Record(row_id, None) # We'll play with schools "Columbia" and "Eureka", which are rows 1,3,5 in the Students table. self.assertTableData("Students", cols="subset", rows="subset", data=[ @@ -680,7 +680,7 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName # A helper for comparing Record objects below. schools_table = self.engine.tables['Schools'] def SchoolsRec(row_id): - return schools_table.Record(schools_table, row_id, None) + return schools_table.Record(row_id, None) # We'll play with schools "Columbia" and "Eureka", which are rows 1,3,5 in the Students table. self.assertTableData("Students", cols="subset", rows="all", data=[ diff --git a/sandbox/grist/test_renames.py b/sandbox/grist/test_renames.py index 9e3b1aa0..81a275ce 100644 --- a/sandbox/grist/test_renames.py +++ b/sandbox/grist/test_renames.py @@ -380,7 +380,7 @@ class TestRenames(test_engine.EngineTestCase): from datetime import date # A helper for comparing Record objects below. people_table = self.engine.tables['People'] - people_rec = lambda row_id: people_table.Record(people_table, row_id, None) + people_rec = lambda row_id: people_table.Record(row_id, None) # Verify the data and calculations are correct. self.assertTableData("Address", cols="all", data=[