chrismclarke
commented
6 months ago It seems that the local variable needs to end in
_list, containing it as a substring is not enough: A variableanswer_list_1is not parsed differently, butanswer_1_listis.
@esmeetewinkel Thanks for looking through all these cases
You're right, I can see the default parser only converts special fields (here meaning sheet columns) depending on their name ending
if (typeof value === "string") {
if (field.endsWith("_list")) {
this.row[field] = parseAppDataListString(value);
}
if (field.endsWith("_collection")) {
this.row[field] = parseAppDataCollectionString(this.row[field]);
}
if (field.endsWith("action_list")) {
this.row[field] = this.row[field]
.map((actionString) => parseAppDataActionString(actionString))
.filter((action) => action !== null);
}
}
// convert google/excel number dates to dates (https://stackoverflow.com/questions/16229494/converting-excel-date-serial-number-to-date-using-javascript)
if (field.endsWith("_date")) {
if (typeof this.row[field] === "number") {
this.row[field] = parseAppDateValue(this.row[field]);
}
}It is then only at a later stage that the template parser handles based on row names, and supports the text included anywhere in the name
if (row.name?.includes("_list")) {
row.value = this.parseTemplateList(row.value);
}
if (row.name?.includes("_collection") && row.value && typeof row.value === "string") {
row.value = parseAppDataCollectionString(row.value);
}So that means right now for any data_list the column must end in _list to be processed as expected, and for any template that defines inline then the row must contain _list somewhere in the name.
Do you think this distinction makes sense/is sensible, or better to also allow data_lists to convert if the column has _list anywhere in the name? I don't think it was considered to carefully when first implemented so open to suggestion
esmeetewinkel
jfmcquade
github-actions[bot]
PR Checklist
TODO
Description
Author Notes
Been a while since taking a closer look at the template parser. A few things noted while working on the PR:
Notes
answer_lists need to be defined as local variables with name including
_listin them somewhere. Not sure if this was previously documented anywhere, I haven't changed the handling just noted that it is the case. See more info about special row name handling in questions section belowFollowing the change all content repos should see git diffs as answer_lists populated fully (not just string partial representation)
One limitation of parsing answer lists in the compiler is harder to determine whether a named list is actually a list or a reference to something else. E.g.
{name: "my_answer_list", value: "@local.some_other_answer_list"}. In order to handle this case I've added a check explicitly for;when specifying template lists. That may break content if a sheet defined a list with only one value and forgot to include, e.g. answer_list local variable value defined as:as opposed to
There is currently a default to handle rows without a name which is just setting the name as the row type. This vaguely works if there is just one text or image type row in a template and nothing needs to modify or refer back, however felt quite fragile in general. I've added a minor update so that by default we now also add a suffix to the name derived from the row number being processed, e.g. if the sheet has unnamed text on row 8 the generated name should be
text_8. Is this something useful, or would it be better to remove entirely and replace with some quality-control check to ensure all rows have an author-defined name.With the changes above there may be small content diffs when syncing repos if any do contain non-named rows
Questions
It appears that parameter lists only ever support simple key-value pairs during parsing (e.g.
answer_list: @local.some_list), with additional support for key-only properties (e.g.input_allowedwould be implied asinput_allowed: 'true'). Do we have any runtime cases of more complex use cases that we might want to reconsider evaluating in the parser? E.g. parameter lists where the value isn't just a simple string but more complex object?There seems support
template_grouprow type, is this something we still use? (don't have strong memory of it)We're currently passing the
exclude_from_translationproperty explicitly with every row entry. Given that this info isn't ever used at runtime it might be worth considering exporting this separately (e.g. a single json file of excluded translations). Possibly a small follow-up issue (might be best to coordinate with translation workflow)Templates have special handling for any variables named
_list(convert to array) or_collection(convert to json key-value pairs). This is on top of specific handling for columns ending_list(e.g. condition_list),_collection,_date, or specificallyaction_list. Is this something documented somewhere? Do we know if things like local name and/or column name_collectionhandling is still used?Dev Notes
I've left the runtime answer list item parse method in the code for now for any cases where the parsing may not have been done correctly in advance (I don't think it should happen but probably better to be safe for now.. maybe if read in from a global or cell within a data_list (as opposed to full data list row)). I've also added an explicit call to sentry to detect if the code is executed to track whether we can safely remove in the future
I've done my best to add spec tests to cover all the cases covered explicitly within the template parser methods in their current state. I'm not sure if it might make sense to further try to refine/improve as follow-up (e.g. remove
template_grouphandling which I think must be deprecated by now). Spec tests can be run via:The code also includes a placeholder method to run specific QC checks on templates, however I stopped short of adding as the PR was already growing quickly. Might be worth reviewing in the future possible checks worth having (e.g. I had planned to include one to ensure all parameter lists that included answer lists pointed at local variables with correct _list in name)
Git Issues
Closes #2210
Screenshots/Videos
Tests added - see full test details in spec file