Skip to content

Commit

Permalink
Improve merging logic
Browse files Browse the repository at this point in the history
  • Loading branch information
ehmicky committed Jan 9, 2022
1 parent c648b42 commit 448a4fc
Show file tree
Hide file tree
Showing 20 changed files with 110 additions and 165 deletions.
23 changes: 7 additions & 16 deletions src/combination/ids/user.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import { getMergeIdsArray } from '../../history/merge/prop.js'
import { getInputIds } from '../inputs.js'

import { getCombsDimensionsIds } from './get.js'

// Retrieve user-defined identifiers
export const getUserIds = function (combinations, inputsList, merge) {
export const getUserIds = function (combinations, inputsList) {
const dimensionsIds = getCombsDimensionsIds(combinations)
.filter(isUserId)
.map(getCombinationUserId)
const nonCombinationsIds = getNonCombinationsIds(inputsList, merge)
const nonCombinationsIds = getNonCombinationsIds(inputsList)
return [...dimensionsIds, ...nonCombinationsIds]
}

Expand All @@ -21,23 +20,15 @@ const getCombinationUserId = function ({ dimension: { messageName }, id }) {
}

// Identifiers that do not relate to dimensions/combinations
const getNonCombinationsIds = function (inputsList, merge) {
const getNonCombinationsIds = function (inputsList) {
return NON_COMBINATION_IDS.flatMap(({ messageName, getIds }) =>
listNonCombinationIds({ messageName, getIds, inputsList, merge }),
listNonCombinationIds({ messageName, getIds, inputsList }),
)
}

const listNonCombinationIds = function ({
messageName,
getIds,
inputsList,
merge,
}) {
const ids = getIds({ inputsList, merge })
const listNonCombinationIds = function ({ messageName, getIds, inputsList }) {
const ids = getIds(inputsList)
return ids.map((id) => ({ messageName, id }))
}

const NON_COMBINATION_IDS = [
{ messageName: 'input', getIds: getInputIds },
{ messageName: '"merge" configuration property', getIds: getMergeIdsArray },
]
const NON_COMBINATION_IDS = [{ messageName: 'input', getIds: getInputIds }]
12 changes: 3 additions & 9 deletions src/combination/ids/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,8 @@ import { getCombsDimensionsIds } from './get.js'
import { getUserIds } from './user.js'

// Validate combination identifiers.
export const validateCombinationsIds = function (
combinations,
inputsList,
merge,
) {
const userIds = getUserIds(combinations, inputsList, merge)
export const validateCombinationsIds = function (combinations, inputsList) {
const userIds = getUserIds(combinations, inputsList)
userIds.forEach(validateUserIds)

const dimensionsIds = getCombsDimensionsIds(combinations)
Expand Down Expand Up @@ -51,9 +47,7 @@ const USER_ID_INVALID_START = '-'
// We do not allow empty strings.
// We do not allow dots because they are used in CLI flags for nested
// configuration properties.
// We forbid other characters:
// - For forward compatibility
// - To use them as filenames (e.g. "merge" identifiers)
// We forbid other characters for forward compatibility.
const USER_ID_REGEXP = /^\w[\w-]*$/u

const validateReservedIds = function (id, messageName) {
Expand Down
2 changes: 1 addition & 1 deletion src/combination/inputs.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const getInputPair = function ({ inputId, inputValue }) {
}

// Retrieve all inputs identifiers
export const getInputIds = function ({ inputsList }) {
export const getInputIds = function (inputsList) {
return inputsList.map(getInputId)
}

Expand Down
3 changes: 1 addition & 2 deletions src/combination/list.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,12 @@ export const listCombinations = async function ({
inputs,
system,
cwd,
merge,
}) {
const tasks = await listTasks(runners, cwd)
const inputsList = toInputsList(inputs)

const combinations = getCombinationsProduct({ tasks, inputsList, system })
validateCombinationsIds(combinations, inputsList, merge)
validateCombinationsIds(combinations, inputsList)
return combinations
}

Expand Down
7 changes: 0 additions & 7 deletions src/config/check.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { isDeepStrictEqual, inspect } from 'util'

import isPlainObj from 'is-plain-obj'
import { validate as isUuid } from 'uuid'

import { UserError } from '../error/main.js'

Expand Down Expand Up @@ -96,9 +95,3 @@ const isJson = function (value) {
return false
}
}

export const checkUuid = function (value, name) {
if (!isUuid(value)) {
throw new UserError(`'${name}' must be a UUID: ${inspect(value)}`)
}
}
6 changes: 3 additions & 3 deletions src/config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { cwd as getCwd } from 'process'

import isPlainObj from 'is-plain-obj'

import { getDefaultId } from '../history/merge/id.js'
import { isTtyInput } from '../report/tty.js'
import { cleanObject } from '../utils/clean.js'

Expand All @@ -11,6 +12,7 @@ export const addDefaultConfig = function (config, command) {
...DEFAULT_CONFIG,
cwd: getCwd(),
force: !isTtyInput(),
merge: getDefaultId(),
showSystem: getDefaultShowSystem(config),
showMetadata: METADATA_COMMANDS.has(command),
...config,
Expand Down Expand Up @@ -53,11 +55,9 @@ const FORCED_CONFIG = {
show: {},
remove: {},
dev: {
// `dev` does not require history.
// As a performance optimization, we use `since: 0` so it is not loaded.
// `dev` does not use history.
delta: 0,
since: 0,
// Merging results does not make with `dev` since the history is not used.
merge: undefined,
},
}
6 changes: 1 addition & 5 deletions src/config/normalize.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { parseLimits } from '../history/compare/parse.js'
import { normalizeMerge } from '../history/merge/id.js'
import { normalizePrecision } from '../run/precision.js'

import {
Expand All @@ -9,7 +10,6 @@ import {
checkDefinedStringArray,
checkDefinedString,
checkJsonObject,
checkUuid,
} from './check.js'

// Normalize configuration shape and do custom validation
Expand Down Expand Up @@ -72,10 +72,6 @@ const normalizeLimit = function (value, propName) {
return { limits }
}

const normalizeMerge = function (value, propName) {
checkUuid(value, propName)
}

const checkTitles = function (value, propName) {
Object.entries(value).forEach(([childName, propValue]) => {
checkDefinedString(propValue, `${propName}.${childName}`)
Expand Down
17 changes: 7 additions & 10 deletions src/history/data/metadata.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,14 @@ import { cleanObject } from '../../utils/clean.js'

// Retrieve filename from a rawResult
export const getRawResultFilename = function (rawResult) {
const { id, mergeId, timestamp } = rawResult
const filename = serializeFilename({ id, mergeId, timestamp })
return filename
const { id, timestamp } = rawResult
return serializeFilename({ id, timestamp })
}

// Retrieve filename from a metadatum
export const serializeFilename = function ({ id, mergeId, timestamp }) {
const mergeIdStr = mergeId === undefined ? '' : `--${mergeId}`
export const serializeFilename = function ({ id, timestamp }) {
const timestampStr = serializeTimestamp(timestamp)
const filename = `${timestampStr}${mergeIdStr}--${id}.json`
return filename
return `${timestampStr}--${id}.json`
}

const serializeTimestamp = function (timestamp) {
Expand All @@ -38,9 +35,9 @@ export const parseFilename = function (filename) {
return
}

const { id, mergeId } = result.groups
const { id } = result.groups
const timestamp = parseTimestamp(result.groups)
return cleanObject({ id, mergeId, timestamp })
return cleanObject({ id, timestamp })
}

const parseTimestamp = function ({
Expand All @@ -58,4 +55,4 @@ const parseTimestamp = function ({
}

const RESULT_FILENAME_REGEXP =
/^(?<year>\d{4})-(?<month>\d{2})-(?<day>\d{2})--(?<hours>\d{2})-(?<minutes>\d{2})-(?<seconds>\d{2})(--(?<mergeId>[\w-]+))?--(?<id>[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12})\.json$/u
/^(?<year>\d{4})-(?<month>\d{2})-(?<day>\d{2})--(?<hours>\d{2})-(?<minutes>\d{2})-(?<seconds>\d{2})--(?<id>[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12})\.json$/u
2 changes: 1 addition & 1 deletion src/history/delta/find.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { findFormat } from './formats/main.js'
// while the `since` delta is relative to the main delta.
// The metadata and history are:
// - Sorted from most to least recent
// - Grouped according to `merge`
// - Grouped by `id`
// When a delta is approximative, the first rawResult after (not before) the
// delta is usually used
// - This allows users to specify loose deltas without errors, such as
Expand Down
12 changes: 5 additions & 7 deletions src/history/delta/formats/id.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,18 @@
import { validate as isUuid } from 'uuid'
import { isValidId } from '../../merge/id.js'

// Deltas can be the `rawResult.id`
const parseId = function (delta) {
if (!isUuid(delta)) {
if (!isValidId(delta)) {
return
}

return delta
}

const findById = function (metadataGroups, id) {
return metadataGroups.findIndex((metadata) => metadataHasId(metadata, id))
}

const metadataHasId = function (metadata, id) {
return metadata.some((metadatum) => metadatum.id === id)
return metadataGroups.findIndex(
([firstMetadatum]) => firstMetadatum.id === id,
)
}

export const idFormat = {
Expand Down
56 changes: 29 additions & 27 deletions src/history/merge/id.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,38 @@
import { groupBy } from '../../utils/group.js'

// `merge` can be "last", which refers to the previous result's mergeId:
// - If there are no previous results, the current result's id is used
// - If the previous result has no mergeId, its id is used
// The new value is persisted in results.
export const normalizeMergeId = function (targetResult, history) {
return targetResult.mergeId === LAST_MERGE_ID
? { ...targetResult, mergeId: replaceMergeIdLast(targetResult, history) }
: targetResult
}
import { inspect } from 'util'

import { v4 as uuidv4, validate as isUuid } from 'uuid'

const LAST_MERGE_ID = 'last'
import { UserError } from '../../error/main.js'

const replaceMergeIdLast = function (targetResult, history) {
if (history.length === 0) {
return targetResult.id
// Validate `merge` property
export const normalizeMerge = function (value, name) {
if (!isValidId(value) && value !== LAST_ID) {
throw new UserError(
`'${name}' must be "${LAST_ID}" or a UUID: ${inspect(value)}`,
)
}
}

const { id, mergeId = id } = history[history.length - 1]
return mergeId
// Validate `result.id`.
// "last" is not persisted since it is normalized first.
export const isValidId = function (value) {
return isUuid(value)
}

// Group either metadata or rawResults by mergeId:
// - They must both have the following properties: `id`, `mergeId`
// `result.mergeId` defaults to `result.id`
// - This allows merging to previous results even when the user did not
// previously intend to
// - However, the default value is assigned at load time, it is not persisted
export const groupByMergeId = function (metadataOrRawResults) {
return Object.values(groupBy(metadataOrRawResults, getMergeId))
// `merge` can be "last", which refers to the previous result's id.
// If there are no previous results, a new UUIDv4 is generated.
export const normalizeId = function (targetResult, history) {
if (targetResult.id !== LAST_ID) {
return targetResult
}

const id =
history.length === 0 ? getDefaultId() : history[history.length - 1].id
return { ...targetResult, id }
}

const getMergeId = function ({ id, mergeId = id }) {
return mergeId
const LAST_ID = 'last'

export const getDefaultId = function () {
return uuidv4()
}
10 changes: 5 additions & 5 deletions src/history/merge/metadata.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import sortOn from 'sort-on'

import { groupByMergeId } from './id.js'
import { groupBy } from '../../utils/group.js'

// Results with the same `mergeId` should be handled like a single result by
// the delta logic. Therefore, we group metadata before applying deltas.
// Results with the same `id` should be handled like a single result by the
// delta logic. Therefore, we group metadata before applying deltas.
export const groupMetadata = function (metadata) {
const metadataA = sortOn(metadata, 'timestamp')
const metadataGroups = groupByMergeId(metadataA)
const metadataGroups = Object.values(groupBy(metadataA, 'id'))
const metadataGroupsA = sortOn(metadataGroups, getMetadataGroupOrder)
return metadataGroupsA
}
Expand All @@ -16,7 +16,7 @@ const getMetadataGroupOrder = function (metadataGroup) {
return lastMetadatum.timestamp
}

// We ungroup metadata before fetching results' contents to abstract the `merge`
// We ungroup metadata before fetching results' contents to abstract the merge
// logic from the store logic.
export const ungroupMetadata = function (metadataGroups) {
return metadataGroups.flat()
Expand Down
14 changes: 0 additions & 14 deletions src/history/merge/prop.js

This file was deleted.

Loading

0 comments on commit 448a4fc

Please sign in to comment.