(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
pull/254/head
Alex Hall 2 years ago
parent 44b4ec7edf
commit 42060df29a

@ -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) {

@ -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=<value>, ...)', True),
'.lookupRecords': Completion('.lookupRecords', '(colName=<value>, ...)', 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=<value>, ...)', True)
self._functions['.lookupRecords'] = Completion('.lookupRecords', '(colName=<value>, ...)', 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. `<refcol>=$id`) and no other reference lookups (i.e. `<refcol>=$<other refcol>`).
"""
# 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

@ -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:

@ -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):
"""

@ -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=<value>, ...)', True),
('Students.lookupRecords', '(colName=<value>, ...)', 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=<value>, ...)', True),
('Max.lookupRecords', '(colName=<value>, ...)', 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=<value>, ...)', True),
('Address.lookupRecords', '(colName=<value>, ...)', 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=<value>, ...)', True),
('Address.lookupRecords', '(colName=<value>, ...)', 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. `<refcol to current table>=$id`,
# but only for `lookupRecords`, not `lookupOne`.
self.assertEqual(
self.engine.autocomplete("stu", "Schools", "name", self.user),
[
'Students',
('Students.lookupOne', '(colName=<value>, ...)', True),
('Students.lookupRecords', '(colName=<value>, ...)', 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=<value>, ...)', True),
('Schools.lookupRecords', '(colName=<value>, ...)', 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=<value>, ...)', True),
('Schools.lookupRecords', '(colName=<value>, ...)', 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=',
],
)

Loading…
Cancel
Save