From 5b352211c4e94d43545d013391dc8c68ac1a806d Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Mon, 21 Feb 2022 16:45:17 +0200 Subject: [PATCH] (core) Guess date format during type conversion Summary: - Adds a dependency moment-guess (https://github.com/apoorv-mishra/moment-guess) to guess date formats from strings. However the npm package is missing source maps which leads to an ugly warning, so currently using a fork until https://github.com/apoorv-mishra/moment-guess/pull/22 is resolved. - Adds guessDateFormat using moment-guess to determine the best candidate date format. The logic may be refined for e.g. lossless imports where the stakes are higher, but for now we're just trying to make type conversions smoother. - Uses guessDateFormat to guess widget options when changing column type to date or datetime. - Uses the date format of the original column when possible instead of guessing. - Fixes a bug where choices were guessed based on the display column instead of the visible column, which made the guessed choices influenced by which values were referenced as well as completely broken when converting from reflist. - @dsagal @georgegevoian This builds on https://phab.getgrist.com/D3265, currently unmerged. That diff was created first to alert to the change. Without it there would still be similar test failures/changes here as the date format would often be concretely guessed and saved as YYYY-MM-DD instead of being left as the default `undefined` which is shows as YYYY-MM-DD in the dropdown. Test Plan: Added a unit test to `parseDate.ts`. Updated several browser tests which show the guessing in action during type conversion quite nicely. Reviewers: georgegevoian Reviewed By: georgegevoian Subscribers: dsagal, georgegevoian Differential Revision: https://phab.getgrist.com/D3264 --- app/client/components/TypeConversion.ts | 22 ++++++-- app/client/components/TypeTransform.ts | 13 +---- app/client/widgets/DateTextBox.js | 15 +----- app/client/widgets/DateTimeTextBox.js | 13 +---- app/common/TableData.ts | 9 +--- app/common/declarations.d.ts | 2 + app/common/gutil.ts | 12 +++++ app/common/parseDate.ts | 70 +++++++++++++++++++++++++ package.json | 1 + test/nbrowser/gristUtils.ts | 6 ++- yarn.lock | 21 ++++++++ 11 files changed, 136 insertions(+), 48 deletions(-) diff --git a/app/client/components/TypeConversion.ts b/app/client/components/TypeConversion.ts index 11cda7da..b3307535 100644 --- a/app/client/components/TypeConversion.ts +++ b/app/client/components/TypeConversion.ts @@ -9,6 +9,7 @@ import {ColumnRec} from 'app/client/models/entities/ColumnRec'; import * as gristTypes from 'app/common/gristTypes'; import {isFullReferencingType} from 'app/common/gristTypes'; import * as gutil from 'app/common/gutil'; +import {dateTimeWidgetOptions, guessDateFormat} from 'app/common/parseDate'; import {TableData} from 'app/common/TableData'; import {decodeObject} from 'app/plugin/objtypes'; @@ -92,8 +93,21 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe formula: "CURRENT_CONVERSION(rec)", }; - const prevOptions = origCol.widgetOptionsJson.peek() || {}; + const visibleCol = origCol.visibleColModel(); + // Column used to derive previous widget options and sample values for guessing + const sourceCol = visibleCol.getRowId() !== 0 ? visibleCol : origCol; + const prevOptions = sourceCol.widgetOptionsJson.peek() || {}; switch (toType) { + case 'Date': + case 'DateTime': { + let {dateFormat} = prevOptions; + if (!dateFormat) { + const colValues = tableData.getColValues(sourceCol.colId()) || []; + dateFormat = guessDateFormat(colValues.map(String)) || "YYYY-MM-DD"; + } + widgetOptions = dateTimeWidgetOptions(dateFormat); + break; + } case 'Choice': { if (Array.isArray(prevOptions.choices)) { // Use previous choices if they are set, e.g. if converting from ChoiceList @@ -101,8 +115,7 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe } else { // Set suggested choices. Limit to 100, since too many choices is more likely to cause // trouble than desired behavior. For many choices, recommend using a Ref to helper table. - const colId = isReferenceCol(origCol) ? origDisplayCol.colId() : origCol.colId(); - const columnData = tableData.getDistinctValues(colId, 100); + const columnData = tableData.getDistinctValues(sourceCol.colId(), 100); if (columnData) { columnData.delete(""); columnData.delete(null); @@ -119,8 +132,7 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe // Set suggested choices. This happens before the conversion to ChoiceList, so we do some // light guessing for likely choices to suggest. const choices = new Set(); - const colId = isReferenceCol(origCol) ? origDisplayCol.colId() : origCol.colId(); - for (let value of tableData.getColValues(colId) || []) { + for (let value of tableData.getColValues(sourceCol.colId()) || []) { if (value === null) { continue; } value = String(decodeObject(value)).trim(); const tags: unknown[] = (value.startsWith('[') && gutil.safeJsonParse(value, null)) || value.split(","); diff --git a/app/client/components/TypeTransform.ts b/app/client/components/TypeTransform.ts index 6655d8e3..3941897d 100644 --- a/app/client/components/TypeTransform.ts +++ b/app/client/components/TypeTransform.ts @@ -15,10 +15,8 @@ import {basicButton, primaryButton} from 'app/client/ui2018/buttons'; import {testId} from 'app/client/ui2018/cssVars'; import {FieldBuilder} from 'app/client/widgets/FieldBuilder'; import {NewAbstractWidget} from 'app/client/widgets/NewAbstractWidget'; -import {ColValues, UserAction} from 'app/common/DocActions'; +import {UserAction} from 'app/common/DocActions'; import {Computed, dom, fromKo, Observable} from 'grainjs'; -import isEmpty = require('lodash/isEmpty'); -import pickBy = require('lodash/pickBy'); // To simplify diff (avoid rearranging methods to satisfy private/public order). /* eslint-disable @typescript-eslint/member-ordering */ @@ -150,14 +148,7 @@ export class TypeTransform extends ColumnTransform { public async setType(toType: string) { const docModel = this.gristDoc.docModel; const colInfo = await TypeConversion.prepTransformColInfo(docModel, this.origColumn, this.origDisplayCol, toType); - // Only update those values which changed, and only if needed. const tcol = this.transformColumn; - const changedInfo = pickBy(colInfo, (val, key) => - (val !== tcol[key as keyof TypeConversion.ColInfo].peek())); - if (!isEmpty(changedInfo)) { - // Update the transform column, particularly the type. - // This will trigger the subscription in postAddTransformColumn and lead to calling convertValues. - await tcol.updateColValues(changedInfo as ColValues); - } + await tcol.updateColValues(colInfo as any); } } diff --git a/app/client/widgets/DateTextBox.js b/app/client/widgets/DateTextBox.js index 87bb985a..2a93bb63 100644 --- a/app/client/widgets/DateTextBox.js +++ b/app/client/widgets/DateTextBox.js @@ -12,6 +12,7 @@ const {cssRow, cssLabel} = require('app/client/ui/RightPanel'); const {cssTextInput} = require("app/client/ui2018/editableLabel"); const {styled, fromKo} = require('grainjs'); const {select} = require('app/client/ui2018/menus'); +const {dateFormatOptions} = require('app/common/parseDate'); /** * DateTextBox - The most basic widget for displaying simple date information. @@ -23,18 +24,6 @@ function DateTextBox(field) { this.dateFormat = this.options.prop('dateFormat'); this.isCustomDateFormat = this.options.prop('isCustomDateFormat'); - this.dateFormatOptions = [ - 'YYYY-MM-DD', - 'MM-DD-YYYY', - 'MM/DD/YYYY', - 'MM-DD-YY', - 'MM/DD/YY', - 'DD MMM YYYY', - 'MMMM Do, YYYY', - 'DD-MM-YYYY', - 'Custom' - ]; - // Helper to set 'dateFormat' and 'isCustomDateFormat' from the set of default date format strings. this.standardDateFormat = this.autoDispose(ko.computed({ owner: this, @@ -58,7 +47,7 @@ DateTextBox.prototype.buildDateConfigDom = function() { var self = this; return dom('div', cssLabel("Date Format"), - cssRow(dom(select(fromKo(self.standardDateFormat), self.dateFormatOptions), dom.testId("Widget_dateFormat"))), + cssRow(dom(select(fromKo(self.standardDateFormat), [...dateFormatOptions, "Custom"]), dom.testId("Widget_dateFormat"))), kd.maybe(self.isCustomDateFormat, function() { return cssRow(dom(textbox(self.dateFormat), dom.testId("Widget_dateCustomFormat"))); }) diff --git a/app/client/widgets/DateTimeTextBox.js b/app/client/widgets/DateTimeTextBox.js index 5ab8ab03..b398b1a4 100644 --- a/app/client/widgets/DateTimeTextBox.js +++ b/app/client/widgets/DateTimeTextBox.js @@ -15,6 +15,7 @@ const {cssTextInput} = require("app/client/ui2018/editableLabel"); const {dom: gdom, styled, fromKo} = require('grainjs'); const {select} = require('app/client/ui2018/menus'); const {buildTZAutocomplete} = require('app/client/widgets/TZAutocomplete'); +const {timeFormatOptions} = require("app/common/parseDate"); /** @@ -32,16 +33,6 @@ function DateTimeTextBox(field) { this.timeFormat = this.options.prop('timeFormat'); this.isCustomTimeFormat = this.options.prop('isCustomTimeFormat'); - this.timeFormatOptions = [ - 'h:mma', - 'h:mma z', - 'HH:mm', - 'HH:mm z', - 'HH:mm:ss', - 'HH:mm:ss z', - 'Custom' - ]; - // Helper to set 'timeFormat' and 'isCustomTimeFormat' from the set of default time format strings. this.standardTimeFormat = this.autoDispose(ko.computed({ owner: this, @@ -72,7 +63,7 @@ DateTimeTextBox.prototype.buildConfigDom = function(isTransformConfig) { ), self.buildDateConfigDom(), cssLabel("Time Format"), - cssRow(dom(select(fromKo(self.standardTimeFormat), self.timeFormatOptions), dom.testId("Widget_timeFormat"))), + cssRow(dom(select(fromKo(self.standardTimeFormat), [...timeFormatOptions, "Custom"]), dom.testId("Widget_timeFormat"))), kd.maybe(self.isCustomTimeFormat, function() { return cssRow(dom(textbox(self.timeFormat), dom.testId("Widget_timeCustomFormat"))); }), diff --git a/app/common/TableData.ts b/app/common/TableData.ts index 360c6817..1f06baad 100644 --- a/app/common/TableData.ts +++ b/app/common/TableData.ts @@ -5,7 +5,7 @@ import {ActionDispatcher} from 'app/common/ActionDispatcher'; import {BulkColValues, CellValue, ColInfo, ColInfoWithId, ColValues, DocAction, isSchemaAction, ReplaceTableData, RowRecord, TableDataAction} from 'app/common/DocActions'; import {getDefaultForType} from 'app/common/gristTypes'; -import {arrayRemove, arraySplice} from 'app/common/gutil'; +import {arrayRemove, arraySplice, getDistinctValues} from 'app/common/gutil'; import {SchemaTypes} from "app/common/schema"; import {UIRowId} from 'app/common/UIRowId'; import isEqual = require('lodash/isEqual'); @@ -231,12 +231,7 @@ export class TableData extends ActionDispatcher implements SkippableRows { public getDistinctValues(colId: string, count: number = Infinity): Set|undefined { const valColumn = this.getColValues(colId); if (!valColumn) { return undefined; } - const distinct: Set = new Set(); - // Add values to the set until it reaches the desired size, or until there are no more values. - for (let i = 0; i < valColumn.length && distinct.size < count; i++) { - distinct.add(valColumn[i]); - } - return distinct; + return getDistinctValues(valColumn, count); } /** diff --git a/app/common/declarations.d.ts b/app/common/declarations.d.ts index 32cc30a6..190e628f 100644 --- a/app/common/declarations.d.ts +++ b/app/common/declarations.d.ts @@ -23,3 +23,5 @@ declare namespace Intl { constructor(locale: string); } } + +declare module '@gristlabs/moment-guess/dist/bundle.js'; diff --git a/app/common/gutil.ts b/app/common/gutil.ts index 9cbdc6bb..3826ed63 100644 --- a/app/common/gutil.ts +++ b/app/common/gutil.ts @@ -915,3 +915,15 @@ export const unwrap: UseCB = (obs: ISubscribable) => { } return (obs as ko.Observable).peek(); }; + +/** + * Get a set of up to `count` distinct values of `values`. + */ +export function getDistinctValues(values: readonly T[], count: number = Infinity): Set { + const distinct = new Set(); + // Add values to the set until it reaches the desired size, or until there are no more values. + for (let i = 0; i < values.length && distinct.size < count; i++) { + distinct.add(values[i]); + } + return distinct; +} diff --git a/app/common/parseDate.ts b/app/common/parseDate.ts index a2463d94..2f17e567 100644 --- a/app/common/parseDate.ts +++ b/app/common/parseDate.ts @@ -1,5 +1,8 @@ import escapeRegExp = require('lodash/escapeRegExp'); import memoize = require('lodash/memoize'); +import {getDistinctValues} from 'app/common/gutil'; +// Simply importing 'moment-guess' inconsistently imports bundle.js or bundle.esm.js depending on environment +import * as guessFormat from '@gristlabs/moment-guess/dist/bundle.js'; import * as moment from 'moment-timezone'; // When using YY format, use a consistent interpretation in datepicker and in moment parsing: add @@ -321,3 +324,70 @@ function standardizeTime(timeString: string): { remaining: string, time: string const hh = String(hours).padStart(2, '0'); return {remaining: timeString.slice(0, match.index).trim(), time: `${hh}:${mm}:${ss}`}; } + +export function guessDateFormat(values: string[], timezone: string = 'UTC'): string | null { + const sample = getDistinctValues(values, 100); + const formats: Record = {}; + for (const dateString of sample) { + let guessed: string | string[]; + try { + guessed = guessFormat(dateString); + } catch { + continue; + } + if (typeof guessed === "string") { + guessed = [guessed]; + } + for (const guess of guessed) { + formats[guess] = 0; + } + } + const formatKeys = Object.keys(formats); + if (!formatKeys.length || formatKeys.length > 10) { + return null; + } + + for (const format of formatKeys) { + for (const dateString of values) { + const m = moment.tz(dateString, format, true, timezone); + if (m.isValid()) { + formats[format] += 1; + } + } + } + + const maxCount = Math.max(...Object.values(formats)); + return formatKeys.find(format => formats[format] === maxCount)!; +} + +export const dateFormatOptions = [ + 'YYYY-MM-DD', + 'MM-DD-YYYY', + 'MM/DD/YYYY', + 'MM-DD-YY', + 'MM/DD/YY', + 'DD MMM YYYY', + 'MMMM Do, YYYY', + 'DD-MM-YYYY', +]; + +export const timeFormatOptions = [ + 'h:mma', + 'h:mma z', + 'HH:mm', + 'HH:mm z', + 'HH:mm:ss', + 'HH:mm:ss z', +]; + +export function dateTimeWidgetOptions(fullFormat: string) { + const index = fullFormat.match(/[hHkaAmsSzZT]|$/)!.index!; + const dateFormat = fullFormat.substr(0, index).trim(); + const timeFormat = fullFormat.substr(index).trim() || timeFormatOptions[0]; + return { + dateFormat, + timeFormat, + isCustomDateFormat: !dateFormatOptions.includes(dateFormat), + isCustomTimeFormat: !timeFormatOptions.includes(timeFormat), + }; +} diff --git a/package.json b/package.json index ef7ef5d9..c230937b 100644 --- a/package.json +++ b/package.json @@ -78,6 +78,7 @@ "@googleapis/oauth2": "0.2.0", "@gristlabs/connect-sqlite3": "0.9.11-grist.1", "@gristlabs/express-session": "1.17.0", + "@gristlabs/moment-guess": "1.2.4-grist.1", "@gristlabs/pidusage": "2.0.17", "@gristlabs/sqlite3": "4.1.1-grist.1", "@popperjs/core": "2.3.3", diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 8089e0e8..27651ccb 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -1789,7 +1789,11 @@ export async function openDocumentSettings() { * Returns date format for date and datetime editor */ export async function getDateFormat(): Promise { - return driver.find('[data-test-id=Widget_dateFormat] .test-select-row').getText(); + const result = await driver.find('[data-test-id=Widget_dateFormat] .test-select-row').getText(); + if (result === "Custom") { + return driver.find('[data-test-id=Widget_dateCustomFormat] input').value(); + } + return result; } /** diff --git a/yarn.lock b/yarn.lock index bd0a3404..1eaa8116 100644 --- a/yarn.lock +++ b/yarn.lock @@ -62,6 +62,14 @@ safe-buffer "5.2.0" uid-safe "~2.1.5" +"@gristlabs/moment-guess@1.2.4-grist.1": + version "1.2.4-grist.1" + resolved "https://registry.yarnpkg.com/@gristlabs/moment-guess/-/moment-guess-1.2.4-grist.1.tgz#b15daba4b9d1e1d4e4fbafbddc0be8dddc21dde3" + integrity sha512-emosrHFak1JYEjZmZoeLO+sjTD48Amx2NH/BRCmiCwH50fcK1VvfwrWHjRu2oQ5p8+yuZxHRxsm0+2KANaNLdg== + dependencies: + arg "^4.1.3" + chalk "^4.1.0" + "@gristlabs/pidusage@2.0.17": version "2.0.17" resolved "https://registry.yarnpkg.com/@gristlabs/pidusage/-/pidusage-2.0.17.tgz#829f77dc9fc711cd4713b774e4fbcc4a71d6cdc5" @@ -780,6 +788,11 @@ are-we-there-yet@~1.1.2: delegates "^1.0.0" readable-stream "^2.0.6" +arg@^4.1.3: + version "4.1.3" + resolved "https://registry.yarnpkg.com/arg/-/arg-4.1.3.tgz#269fc7ad5b8e42cb63c896d5666017261c144089" + integrity sha512-58S9QDqG0Xx27YwPSt9fJxivjYl432YCwfDMfZ+71RAqUrZef7LrKQZ3LHLOwCS4FLNBplP533Zx895SeOCHvA== + argparse@^1.0.7: version "1.0.10" resolved "https://registry.yarnpkg.com/argparse/-/argparse-1.0.10.tgz#bcd6791ea5ae09725e17e5ad988134cd40b3d911" @@ -1529,6 +1542,14 @@ chalk@^4.0.0: ansi-styles "^4.1.0" supports-color "^7.1.0" +chalk@^4.1.0: + version "4.1.2" + resolved "https://registry.yarnpkg.com/chalk/-/chalk-4.1.2.tgz#aac4e2b7734a740867aeb16bf02aad556a1e7a01" + integrity sha512-oKnbhFyRIXpUuez8iBMmyEa4nbj4IOQyuhc/wy9kY7/WVPcwIO9VA668Pu8RkO7+0G76SLROeyw9CpQ061i4mA== + dependencies: + ansi-styles "^4.1.0" + supports-color "^7.1.0" + check-error@^1.0.2: version "1.0.2" resolved "https://registry.yarnpkg.com/check-error/-/check-error-1.0.2.tgz#574d312edd88bb5dd8912e9286dd6c0aed4aac82"