From 42060df29a81542b2bb3df27d7bb4495a2df2f40 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Tue, 16 Aug 2022 21:18:19 +0200 Subject: [PATCH] (core) Formula autocomplete improvements for references and lookups Summary: Makes the following improvements to formula autocomplete: - When a user types `$RefCol` (or part of it), also show `$RefCol.VisibleCol` (replace actual column names) in the autocomplete even before the `.` is typed, to help users understand the difference between a raw reference/record and its visible column. - When a user types a table name, show `.lookupOne` and `.lookupRecords` in the autocomplete, again even before the `.` is typed. - For `.lookupRecords(` and `.lookupOne(`, once the `(` is entered, suggest each column name as a keyword argument. - Also suggest lookup arguments involving compatible reference columns, especially 'reverse reference' lookups like `refcol=$id` which are very common and difficult for users. - To support these features, the Ace editor autocomplete needs some patching to fetch fresh autocomplete options after typing `.` or `(`. This also improves unrelated behaviour that wasn't great before when one column name is contained in another. See the first added browser test. Discussions: - https://grist.slack.com/archives/CDHABLZJT/p1659707068383179 - https://grist.quip.com/HoSmAlvFax0j#MbTADAH5kgG - https://grist.quip.com/HoSmAlvFax0j/Formula-Improvements#temp:C:MbT3649fe964a184e8dada9bbebb Test Plan: Added Python and nbrowser tests. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3580 --- app/client/components/AceEditorCompletions.ts | 39 ++++- sandbox/grist/autocomplete_context.py | 73 +++++++- sandbox/grist/engine.py | 45 ++++- sandbox/grist/table.py | 47 ++--- sandbox/grist/test_completion.py | 164 +++++++++++++++++- 5 files changed, 325 insertions(+), 43 deletions(-) diff --git a/app/client/components/AceEditorCompletions.ts b/app/client/components/AceEditorCompletions.ts index e2099457..1cef17e5 100644 --- a/app/client/components/AceEditorCompletions.ts +++ b/app/client/components/AceEditorCompletions.ts @@ -27,6 +27,40 @@ export function setupAceEditorCompletions(editor: ace.Editor, options: ICompleti completer.autoSelect = false; (editor as any).completer = completer; + // Patch updateCompletions and insertMatch so that fresh completions are fetched when the user types '.' or '(' + const originalUpdate = completer.updateCompletions.bind(completer); + completer.updateCompletions = function(this: any, keepPopupPosition: boolean) { + // The next three lines are copied from updateCompletions() in the ace autocomplete source code. + if (keepPopupPosition && this.base && this.completions) { + const pos = this.editor.getCursorPosition(); + const prefix = this.editor.session.getTextRange({start: this.base, end: pos}); + // If the cursor is just after '.' or '(', prevent this same block from running + // in the original updateCompletions() function. Otherwise it will just keep any remaining completions that match, + // or not show any completions at all. + // But the last character implies that the set of completions is likely to have changed. + if (prefix.endsWith(".") || prefix.endsWith("(")) { + this.completions = null; + } + } + return originalUpdate(keepPopupPosition); + }.bind(completer); + + // Similar patch to the above. + const originalInsertMatch = completer.insertMatch.bind(completer); + completer.insertMatch = function(this: any) { + const base = this.base; // this.base may become null after the next line, save it now. + const result = originalInsertMatch.apply(...arguments); + // Like in the above patch, get the current text in the editor to be completed. + const pos = this.editor.getCursorPosition(); + const prefix = this.editor.session.getTextRange({start: base, end: pos}); + // This patch is specifically for when a previous completion is inserted by pressing Enter/Tab, + // and such completions may end in '(', which can lead to more completions, e.g. for `.lookupRecords(`. + if (prefix.endsWith("(")) { + this.showPopup(this.editor); + } + return result; + }.bind(completer); + aceCompleterAddHelpLinks(completer); // Explicitly destroy the auto-completer on disposal, since it doesn't not remove the element @@ -55,8 +89,9 @@ function initCustomCompleter() { aceLanguageTools.addCompleter({ // Default regexp stops at periods, which doesn't let autocomplete // work on members. So we expand it to include periods. - // We also include $, which grist uses for column names. - identifierRegexps: [/[a-zA-Z_0-9.$\u00A2-\uFFFF]/], + // We also include $, which grist uses for column names, + // and '(' for the start of a function call, which may provide completions for arguments. + identifierRegexps: [/[a-zA-Z_0-9.$\u00A2-\uFFFF(]/], // For autocompletion we ship text to the sandbox and run standard completion there. async getCompletions(editor: ace.Editor, session: ace.IEditSession, pos: number, prefix: string, callback: any) { diff --git a/sandbox/grist/autocomplete_context.py b/sandbox/grist/autocomplete_context.py index 64a3e359..cc1a1fd0 100644 --- a/sandbox/grist/autocomplete_context.py +++ b/sandbox/grist/autocomplete_context.py @@ -5,10 +5,14 @@ It's intended to use with rlcompleter.Completer. It allows finding global names lowercase searches, and adds function usage information to some results. """ import inspect -from collections import namedtuple +import re +from collections import namedtuple, defaultdict from six.moves import builtins import six +import column +from table import UserTable + # funcname is the function name, e.g. "MAX" # argspec is the signature, e.g. "(arg, *more_args)" # isgrist is a boolean for whether this function should be in Grist documentation. @@ -39,17 +43,27 @@ class AutocompleteContext(object): # Prepare detailed Completion objects for functions where we can supply more info. # TODO It would be nice to include builtin functions too, but getargspec doesn't work there. - self._functions = {} + self._functions = { + # Add in the important UserTable methods, with custom friendlier descriptions. + '.lookupOne': Completion('.lookupOne', '(colName=, ...)', True), + '.lookupRecords': Completion('.lookupRecords', '(colName=, ...)', True), + '.Record': Completion('.Record', '', True), + '.RecordSet': Completion('.RecordSet', '', True), + } for key, value in six.iteritems(self._context): if value and callable(value): argspec = inspect.formatargspec(*inspect.getargspec(value)) self._functions[key] = Completion(key, argspec, is_grist_func(value)) - # 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) + for key, value in self._context.copy().items(): + if isinstance(value, UserTable): + for func in [".lookupOne", ".lookupRecords"]: + # Add fake variable names like `Table1.lookupOne` to the context. + # This allows the method to be suggested + # even before the user finishes typing the table name. + # Such a variable name isn't actually possible, so it doesn't matter what value we set. + self._context[key + func] = None + self._functions[key + func] = self._functions[func]._replace(funcname=key + func) # Remember the original name for each lowercase one. self._lowercase = {} @@ -84,6 +98,15 @@ class AutocompleteContext(object): # 'for' suggests the autocompletion 'for ' in python 3 result = result.rstrip() + # Table.lookup methods are special to allow completion just from the table name. + match = re.search(r'\w+\.(lookupOne|lookupRecords)$', result, re.IGNORECASE) + if match: + funcname = match.group().lower() + funcname = self._lowercase.get(match, funcname) + func = self._functions.get(funcname) + if func: + return tuple(func) + # Callables are returned by rlcompleter with a trailing "(". if result.endswith('('): funcname = result[0:-1] @@ -103,3 +126,39 @@ class AutocompleteContext(object): # Return translation from lowercase if there is one, or the result string otherwise. return self._lowercase.get(result, result) + + +def lookup_autocomplete_options(lookup_table, formula_table, reverse_only): + """ + Returns a list of strings to add to `Table.lookupRecords(` (or lookupOne) + to suggest arguments for the method. + `lookup_table` is the table that the method is being called on. + `formula_table` is the table that the formula is being written in. + `reverse_only` should be True to only suggest 'reverse reference' lookup arguments + (i.e. `=$id`) and no other reference lookups (i.e. `=$`). + """ + # dict mapping tables to lists of col_ids in `formula_table` that are references + # to the the table with that table_id. + # In particular `$id` is treated as a reference to `formula_table`. + ref_cols = defaultdict(list, {formula_table: ["id"]}) + if not reverse_only: + for col_id, col in formula_table.all_columns.items(): + # Note that we can't support reflist columns in the current table, + # as there is no `IN()` function to do the opposite of the `CONTAINS()` function. + if isinstance(col, column.ReferenceColumn) and column.is_user_column(col_id): + ref_cols[col._target_table].append(col_id) + + # Find referencing columns in the lookup table that target tables in ref_cols. + results = [] + for lookup_col_id, lookup_col in lookup_table.all_columns.items(): + if isinstance(lookup_col, column.ReferenceColumn): + value_template = "${}" + elif isinstance(lookup_col, column.ReferenceListColumn): + value_template = "CONTAINS(${})" + else: + continue + target_table_id = lookup_col._target_table + for ref_col_id in ref_cols[target_table_id]: + value = value_template.format(ref_col_id) + results.append("{}={})".format(lookup_col_id, value)) + return results diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index d968e94a..ac2bc9e2 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -19,7 +19,7 @@ from sortedcontainers import SortedSet import acl import actions import action_obj -from autocomplete_context import AutocompleteContext +from autocomplete_context import AutocompleteContext, lookup_autocomplete_options from codebuilder import DOLLAR_REGEX import depend import docactions @@ -1418,12 +1418,33 @@ class Engine(object): """ Return a list of suggested completions of the python fragment supplied. """ + table = self.tables[table_id] + + # Table.lookup methods are special to suggest arguments after '(' + match = re.match(r"(\w+)\.(lookupRecords|lookupOne)\($", txt) + if match: + # Get the 'Table1' in 'Table1.lookupRecords(' + lookup_table_id = match.group(1) + if lookup_table_id in self.tables: + lookup_table = self.tables[lookup_table_id] + # Add a keyword argument with no value for each column name in the lookup table. + result = [ + txt + col_id + "=" + for col_id in lookup_table.all_columns + if column.is_user_column(col_id) or col_id == 'id' + ] + # Add specific complete lookups involving reference columns. + result += [ + txt + option + for option in lookup_autocomplete_options(lookup_table, table, reverse_only=False) + ] + return sorted(result) + # replace $ with rec. and add a dummy rec object tweaked_txt = DOLLAR_REGEX.sub(r'rec.', txt) # convert a bare $ with nothing after it also if txt == '$': tweaked_txt = 'rec.' - table = self.tables[table_id] autocomplete_context = self.autocomplete_context context = autocomplete_context.get_context() @@ -1433,10 +1454,10 @@ class Engine(object): context.pop('value', None) context.pop('user', None) - column = table.get_column(column_id) if table.has_column(column_id) else None - if column and not column.is_formula(): + col = table.get_column(column_id) if table.has_column(column_id) else None + if col and not col.is_formula(): # Add trigger formula completions. - context['value'] = column.sample_value() + context['value'] = col.sample_value() context['user'] = User(user, self.tables, is_sample=True) completer = rlcompleter.Completer(context) @@ -1450,7 +1471,19 @@ class Engine(object): break if skipped_completions.search(result): continue - results.append(autocomplete_context.process_result(result)) + result = autocomplete_context.process_result(result) + results.append(result) + funcname = result[0] + # Suggest reverse reference lookups, specifically only for .lookupRecords(), + # not for .lookupOne(). + if isinstance(result, tuple) and funcname.endswith(".lookupRecords"): + lookup_table_id = funcname.split(".")[0] + if lookup_table_id in self.tables: + lookup_table = self.tables[lookup_table_id] + results += [ + funcname + "(" + option + for option in lookup_autocomplete_options(lookup_table, table, reverse_only=True) + ] # If we changed the prefix (expanding the $ symbol) we now need to change it back. if tweaked_txt != txt: diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index 50802201..421284a5 100644 --- a/sandbox/grist/table.py +++ b/sandbox/grist/table.py @@ -18,22 +18,6 @@ import usertypes log = logger.Logger(__name__, logger.INFO) -def _make_sample_record(table_id, col_objs): - """ - Helper to create a sample record for a table, used for auto-completions. - """ - # This type gets created with a property for each column. We use property-methods rather than - # plain properties because this sample record is created before all tables have initialized, so - # reference values (using .sample_record for other tables) are not yet available. - RecType = type(table_id, (), { - # Note col=col to bind col at lambda-creation time; see - # https://stackoverflow.com/questions/10452770/python-lambdas-binding-to-local-values - col.col_id: property(lambda self, col=col: col.sample_value()) - for col in col_objs - if column.is_user_column(col.col_id) or col.col_id == 'id' - }) - return RecType() - def get_default_func_name(col_id): return "_default_" + col_id @@ -252,10 +236,33 @@ class Table(object): """ Used for auto-completion as a record with correct properties of correct types. """ - return _make_sample_record( - self.table_id, - [col for col_id, col in self.all_columns.items() if col_id not in self._special_cols], - ) + # Create a type with a property for each column. We use property-methods rather than + # plain attributes because this sample record is created before all tables have initialized, so + # reference values (using .sample_record for other tables) are not yet available. + props = {} + for col in self.all_columns.values(): + if not (column.is_user_column(col.col_id) or col.col_id == 'id'): + continue + # Note c=col to bind at lambda-creation time; see + # https://stackoverflow.com/questions/10452770/python-lambdas-binding-to-local-values + props[col.col_id] = property(lambda _self, c=col: c.sample_value()) + if col.col_id == 'id': + # The column lookup below doesn't work for the id column + continue + # For columns with a visible column (i.e. most Reference/ReferenceList columns), + # we also want to show how to get that visible column instead of the 'raw' record + # returned by the reference column itself. + col_rec = self._engine.docmodel.get_column_rec(self.table_id, col.col_id) + visible_col_id = col_rec.visibleCol.colId + if visible_col_id: + # This creates a fake attribute like `RefCol.VisibleCol` which isn't valid syntax normally, + # to show the `.VisibleCol` part before the user has typed the `.` + props[col.col_id + "." + visible_col_id] = property( + lambda _self, c=col, v=visible_col_id: getattr(c.sample_value(), v) + ) + + RecType = type(self.table_id, (), props) + return RecType() def _rebuild_model(self, user_table): """ diff --git a/sandbox/grist/test_completion.py b/sandbox/grist/test_completion.py index 4a3aac54..cf22235b 100644 --- a/sandbox/grist/test_completion.py +++ b/sandbox/grist/test_completion.py @@ -20,7 +20,8 @@ class TestCompletion(test_engine.EngineTestCase): self.load_sample(testsamples.sample_students) # To test different column types, we add some differently-typed columns to the sample. - self.add_column('Students', 'school', type='Ref:Schools') + self.add_column('Students', 'school', type='Ref:Schools', visibleCol=10) + self.add_column('Students', 'homeAddress', type='Ref:Address', visibleCol=21) self.add_column('Students', 'birthDate', type='Date') self.add_column('Students', 'lastVisit', type='DateTime:America/New_York') self.add_column('Schools', 'yearFounded', type='Int') @@ -102,10 +103,13 @@ class TestCompletion(test_engine.EngineTestCase): [ 'user.StudentInfo.birthDate', 'user.StudentInfo.firstName', + 'user.StudentInfo.homeAddress', + 'user.StudentInfo.homeAddress.city', 'user.StudentInfo.id', 'user.StudentInfo.lastName', 'user.StudentInfo.lastVisit', 'user.StudentInfo.school', + 'user.StudentInfo.school.name', 'user.StudentInfo.schoolCities', 'user.StudentInfo.schoolIds', 'user.StudentInfo.schoolName' @@ -164,8 +168,15 @@ class TestCompletion(test_engine.EngineTestCase): ('STDEVP', '(value, *more_values)', True), ('STDEVPA', '(value, *more_values)', True) ]) - self.assertEqual(self.engine.autocomplete("stu", "Address", "city", self.user), - ["Students"]) + self.assertEqual( + self.engine.autocomplete("stu", "Address", "city", self.user), + [ + 'Students', + ('Students.lookupOne', '(colName=, ...)', True), + ('Students.lookupRecords', '(colName=, ...)', True), + 'Students.lookupRecords(homeAddress=$id)', + ], + ) # Add a table name whose lowercase version conflicts with a builtin. self.apply_user_action(['AddTable', 'Max', []]) @@ -173,6 +184,8 @@ class TestCompletion(test_engine.EngineTestCase): ('MAX', '(value, *more_values)', True), ('MAXA', '(value, *more_values)', True), 'Max', + ('Max.lookupOne', '(colName=, ...)', True), + ('Max.lookupRecords', '(colName=, ...)', True), 'max(', ]) self.assertEqual(self.engine.autocomplete("MAX", "Address", "city", self.user), [ @@ -185,7 +198,14 @@ class TestCompletion(test_engine.EngineTestCase): # Should suggest globals and table names. self.assertEqual(self.engine.autocomplete("ME", "Address", "city", self.user), [('MEDIAN', '(value, *more_values)', True)]) - self.assertEqual(self.engine.autocomplete("Ad", "Address", "city", self.user), ['Address']) + self.assertEqual( + self.engine.autocomplete("Ad", "Address", "city", self.user), + [ + 'Address', + ('Address.lookupOne', '(colName=, ...)', True), + ('Address.lookupRecords', '(colName=, ...)', True), + ], + ) self.assertGreaterEqual(set(self.engine.autocomplete("S", "Address", "city", self.user)), { 'Schools', 'Students', @@ -199,7 +219,14 @@ class TestCompletion(test_engine.EngineTestCase): ('SUM', '(value1, *more_values)', True), ('STDEV', '(value, *more_values)', True), }) - self.assertEqual(self.engine.autocomplete("Addr", "Schools", "budget", self.user), ['Address']) + self.assertEqual( + self.engine.autocomplete("Addr", "Schools", "budget", self.user), + [ + 'Address', + ('Address.lookupOne', '(colName=, ...)', True), + ('Address.lookupRecords', '(colName=, ...)', True), + ], + ) def test_suggest_columns(self): self.assertEqual(self.engine.autocomplete("$ci", "Address", "city", self.user), @@ -211,12 +238,13 @@ class TestCompletion(test_engine.EngineTestCase): # A few more detailed examples. self.assertEqual(self.engine.autocomplete("$", "Students", "school", self.user), - ['$birthDate', '$firstName', '$id', '$lastName', '$lastVisit', - '$school', '$schoolCities', '$schoolIds', '$schoolName']) + ['$birthDate', '$firstName', '$homeAddress', '$homeAddress.city', + '$id', '$lastName', '$lastVisit', + '$school', '$school.name', '$schoolCities', '$schoolIds', '$schoolName']) self.assertEqual(self.engine.autocomplete("$fi", "Students", "birthDate", self.user), ['$firstName']) self.assertEqual(self.engine.autocomplete("$school", "Students", "lastVisit", self.user), - ['$school', '$schoolCities', '$schoolIds', '$schoolName']) + ['$school', '$school.name', '$schoolCities', '$schoolIds', '$schoolName']) def test_suggest_lookup_methods(self): # Should suggest lookup formulas for tables. @@ -305,3 +333,123 @@ class TestCompletion(test_engine.EngineTestCase): self.engine.autocomplete("$school.address.city.st", "Students", "lastName", self.user), ['$school.address.city.startswith(', '$school.address.city.strip('] ) + + def test_suggest_lookup_early(self): + # For part of a table name, suggest lookup methods early, + # including a 'reverse reference' lookup, i.e. `=$id`, + # but only for `lookupRecords`, not `lookupOne`. + self.assertEqual( + self.engine.autocomplete("stu", "Schools", "name", self.user), + [ + 'Students', + ('Students.lookupOne', '(colName=, ...)', True), + ('Students.lookupRecords', '(colName=, ...)', True), + # i.e. Students.school is a reference to Schools + 'Students.lookupRecords(school=$id)', + ], + ) + self.assertEqual( + self.engine.autocomplete("scho", "Address", "city", self.user), + [ + 'Schools', + ('Schools.lookupOne', '(colName=, ...)', True), + ('Schools.lookupRecords', '(colName=, ...)', True), + # i.e. Schools.address is a reference to Address + 'Schools.lookupRecords(address=$id)', + ], + ) + + # Same as above, but the formula is being entered in 'Students' instead of 'Address', + # which means there's no reverse reference to suggest. + self.assertEqual( + self.engine.autocomplete("scho", "Students", "firstName", self.user), + [ + 'Schools', + ('Schools.lookupOne', '(colName=, ...)', True), + ('Schools.lookupRecords', '(colName=, ...)', True), + ], + ) + + def test_suggest_lookup_arguments(self): + # Typing in the full `.lookupRecords(` should suggest keyword argument (i.e. column) names, + # in addition to reference lookups, including the reverse reference lookups above. + self.assertEqual( + self.engine.autocomplete("Schools.lookupRecords(", "Address", "city", self.user), + [ + 'Schools.lookupRecords(address=', + 'Schools.lookupRecords(address=$id)', + 'Schools.lookupRecords(budget=', + 'Schools.lookupRecords(id=', + 'Schools.lookupRecords(lastModified=', + 'Schools.lookupRecords(lastModifier=', + 'Schools.lookupRecords(name=', + 'Schools.lookupRecords(yearFounded=', + ], + ) + + # In addition to reverse reference lookups, suggest other lookups involving two reference + # columns (one from the looked up table, one from the current table) targeting the same table, + # e.g. `address=$homeAddress` in the two cases below. + self.assertEqual( + self.engine.autocomplete("Schools.lookupRecords(", "Students", "firstName", self.user), + [ + 'Schools.lookupRecords(address=', + 'Schools.lookupRecords(address=$homeAddress)', + 'Schools.lookupRecords(budget=', + 'Schools.lookupRecords(id=', + 'Schools.lookupRecords(lastModified=', + 'Schools.lookupRecords(lastModifier=', + 'Schools.lookupRecords(name=', + 'Schools.lookupRecords(yearFounded=', + ], + ) + + self.assertEqual( + self.engine.autocomplete("Students.lookupRecords(", "Schools", "name", self.user), + [ + 'Students.lookupRecords(birthDate=', + 'Students.lookupRecords(firstName=', + 'Students.lookupRecords(homeAddress=', + 'Students.lookupRecords(homeAddress=$address)', + 'Students.lookupRecords(id=', + 'Students.lookupRecords(lastName=', + 'Students.lookupRecords(lastVisit=', + 'Students.lookupRecords(school=', + 'Students.lookupRecords(school=$id)', + 'Students.lookupRecords(schoolCities=', + 'Students.lookupRecords(schoolIds=', + 'Students.lookupRecords(schoolName=', + ], + ) + + # Add some more reference columns to test that all combinations are offered + self.add_column('Students', 'homeAddress2', type='Ref:Address') + self.add_column('Schools', 'address2', type='Ref:Address') + # This leads to `Students.lookupRecords(moreAddresses=CONTAINS($address[2]))` + self.add_column('Students', 'moreAddresses', type='RefList:Address') + # This doesn't affect anything, because there's no way to do the opposite of CONTAINS() + self.add_column('Schools', 'otherAddresses', type='RefList:Address') + self.assertEqual( + self.engine.autocomplete("Students.lookupRecords(", "Schools", "name", self.user), + [ + 'Students.lookupRecords(birthDate=', + 'Students.lookupRecords(firstName=', + 'Students.lookupRecords(homeAddress2=', + 'Students.lookupRecords(homeAddress2=$address)', + 'Students.lookupRecords(homeAddress2=$address2)', + 'Students.lookupRecords(homeAddress=', + 'Students.lookupRecords(homeAddress=$address)', + 'Students.lookupRecords(homeAddress=$address2)', + 'Students.lookupRecords(id=', + 'Students.lookupRecords(lastName=', + 'Students.lookupRecords(lastVisit=', + 'Students.lookupRecords(moreAddresses=', + 'Students.lookupRecords(moreAddresses=CONTAINS($address))', + 'Students.lookupRecords(moreAddresses=CONTAINS($address2))', + 'Students.lookupRecords(school=', + 'Students.lookupRecords(school=$id)', + 'Students.lookupRecords(schoolCities=', + 'Students.lookupRecords(schoolIds=', + 'Students.lookupRecords(schoolName=', + ], + )