From 37c3ce675927bea60a99fad12bc2c7ab4a3a6c0f Mon Sep 17 00:00:00 2001 From: Eemeli Aro Date: Sun, 28 Mar 2021 19:36:26 +0300 Subject: [PATCH 1/5] feat!: Split flow collections into items in Parser BREAKING CHANGE: The shape of FlowCollection items changes from `Token[]` to `{ start, key, sep, value }[]`, so that it now matches the items of BlockMap and BlockSequence. This means that the corresponding composer is completely rewritten. --- docs/07_parsing_yaml.md | 11 +- docs/08_errors.md | 2 +- src/compose/resolve-flow-collection.ts | 336 ++++++++++--------------- src/compose/resolve-props.ts | 33 ++- src/compose/util-contains-newline.ts | 22 +- src/errors.ts | 2 +- src/parse/parser.ts | 62 ++++- src/parse/tokens.ts | 14 +- tests/doc/errors.js | 34 ++- tests/doc/parse.js | 6 +- 10 files changed, 258 insertions(+), 264 deletions(-) diff --git a/docs/07_parsing_yaml.md b/docs/07_parsing_yaml.md index b8df5a2c..2176eb56 100644 --- a/docs/07_parsing_yaml.md +++ b/docs/07_parsing_yaml.md @@ -187,10 +187,17 @@ Some of the most common node properties include: | `offset` | `number` | The start index within the source string or character stream. | | `source` | `string` | A raw string representation of the node's value, including all newlines and indentation. | | `indent` | `number` | The indent level of the current line; mostly just for internal use. | -| `items` | `{ ... }[]` | The contents of a collection; shape depends on the collection type, and may include `key: Token` and `value: Token`. | +| `items` | `Item[]` | The contents of a collection; exact shape depends on the collection type. | | `start`, `sep`, `end` | `SourceToken[]` | Content before, within, and after "actual" values. Includes item and collection indicators, anchors, tags, comments, as well as other things. | -As an implementation detail, block and flow collections are parsed and presented rather differently due to their structural differences. +Collection items contain some subset of the following properties: + +| Item property | Type | Description | +| ------------- | --------------- | -------------------------------------------------------------------------------------------------------------------------- | +| `start` | `SourceToken[]` | Always defined. Content before the actual value. May include comments that are later assigned to the preceding item. | +| `key` | `Token ⎮ null` | Set for key/value pairs only, so never used in block sequences. | +| `sep` | `SourceToken[]` | Content between the key and the value. If defined, indicates that the `key` logically exists, even if its value is `null`. | +| `value` | `Token ⎮ null` | The value. Normally set, but may be left out for e.g. explicit keys with no matching value. | ### Counting Lines diff --git a/docs/08_errors.md b/docs/08_errors.md index 0a67920c..28e95920 100644 --- a/docs/08_errors.md +++ b/docs/08_errors.md @@ -28,6 +28,7 @@ To identify errors for special handling, you should primarily use `code` to diff | `BAD_DIRECTIVE` | Only the `%YAML` and `%TAG` directives are supported, and they need to follow the specified strucutre. | | `BAD_DQ_ESCAPE` | Double-quotes strings may include `\` escaped content, but that needs to be valid. | | `BAD_INDENT` | Indentation is important in YAML, and collection items need to all start at the same level. Block scalars are also picky about their leading content. | +| `BAD_PROP_ORDER` | Anchors and tags must be placed after the `?`, `:` and `-` indicators. | | `BAD_SCALAR_START` | Plain scalars cannot start with a block scalar indicator, or one of the two reserved characters: `@` and `. To fix, use a block or quoted scalar for the value. | | `BLOCK_AS_IMPLICIT_KEY` | There's probably something wrong with the indentation, or you're trying to parse something like `a: b: c`, where it's not clear what's the key and what's the value. | | `BLOCK_IN_FLOW` | YAML scalars and collections both have block and flow styles. Flow is allowed within block, but not the other way around. | @@ -40,7 +41,6 @@ To identify errors for special handling, you should primarily use `code` to diff | `MULTIPLE_ANCHORS` | A node is only allowed to have one anchor. | | `MULTIPLE_DOCS` | A YAML stream may include multiple documents. If yours does, you'll need to use `parseAllDocuments()` to work with it. | | `MULTIPLE_TAGS` | A node is only allowed to have one tag. | -| `PROP_BEFORE_SEP` | For an explicit key, anchors and tags must be after the `?` indicator | | `TAB_AS_INDENT` | Only spaces are allowed as indentation. | | `TAG_RESOLVE_FAILED` | Something went wrong when resolving a node's tag with the current schema. | | `UNEXPECTED_TOKEN` | A token was encountered in a place where it wasn't expected. | diff --git a/src/compose/resolve-flow-collection.ts b/src/compose/resolve-flow-collection.ts index bd142de1..ed248ef6 100644 --- a/src/compose/resolve-flow-collection.ts +++ b/src/compose/resolve-flow-collection.ts @@ -1,11 +1,11 @@ -import { isNode, isPair, ParsedNode } from '../nodes/Node.js' import { Pair } from '../nodes/Pair.js' import { YAMLMap } from '../nodes/YAMLMap.js' import { YAMLSeq } from '../nodes/YAMLSeq.js' -import type { FlowCollection, SourceToken, Token } from '../parse/tokens.js' +import type { FlowCollection } from '../parse/tokens.js' import type { ComposeContext, ComposeNode } from './compose-node.js' import type { ComposeErrorHandler } from './composer.js' import { resolveEnd } from './resolve-end.js' +import { resolveProps } from './resolve-props.js' import { containsNewline } from './util-contains-newline.js' export function resolveFlowCollection( @@ -15,244 +15,164 @@ export function resolveFlowCollection( onError: ComposeErrorHandler ) { const isMap = fc.start.source === '{' + const fcName = isMap ? 'flow map' : 'flow sequence' const coll = isMap ? new YAMLMap(ctx.schema) : new YAMLSeq(ctx.schema) coll.flow = true - let key: ParsedNode | null = null - let value: ParsedNode | null = null - - let spaceBefore = false - let comment = '' - let hasSpace = false - let newlines = '' - let anchor = '' - let tagName = '' - - let offset = fc.offset + 1 - let atLineStart = false - let atExplicitKey = false - let atValueEnd = false - let nlAfterValueInSeq = false - let seqKeyToken: Token | null = null - - function getProps() { - const props = { spaceBefore, comment, anchor, tagName } - - spaceBefore = false - comment = '' - newlines = '' - anchor = '' - tagName = '' - - return props - } - - function addItem(pos: number) { - if (value) { - if (comment) value.comment = comment - } else { - value = composeEmptyNode(ctx, offset, fc.items, pos, getProps(), onError) - } - if (isMap || atExplicitKey) { - coll.items.push(key ? new Pair(key, value) : new Pair(value)) - } else { - const seq = coll as YAMLSeq - if (key) { - const map = new YAMLMap(ctx.schema) - map.flow = true - map.items.push(new Pair(key, value)) - seq.items.push(map) - } else seq.items.push(value) - } - } - + let offset = fc.offset for (let i = 0; i < fc.items.length; ++i) { - const token = fc.items[i] - let isSourceToken = true - switch (token.type) { - case 'space': - hasSpace = true - break - case 'comment': { - if (ctx.options.strict && !hasSpace) + const { start, key, sep, value } = fc.items[i] + const props = resolveProps(start, { + ctx, + flow: fcName, + indicator: 'explicit-key-ind', + offset, + onError, + startOnNewline: false + }) + if (!props.found) { + if (!props.anchor && !props.tagName && !sep && !value) { + if (i === 0 && props.comma) onError( - offset, - 'COMMENT_SPACE', - 'Comments must be separated from other tokens by white space characters' + props.comma.offset, + 'UNEXPECTED_TOKEN', + `Unexpected , in ${fcName}` ) - const cb = token.source.substring(1) - if (!comment) comment = cb - else comment += newlines + cb - atLineStart = false - newlines = '' - break - } - case 'newline': - if (atLineStart && !comment) spaceBefore = true - if (atValueEnd) { - if (comment) { - let node = coll.items[coll.items.length - 1] - if (isPair(node)) node = node.value || node.key - /* istanbul ignore else should not happen */ - if (isNode(node)) node.comment = comment - else - onError( - offset, - 'IMPOSSIBLE', - 'Error adding trailing comment to node' - ) - comment = '' - } - atValueEnd = false - } else { - newlines += token.source - if (!isMap && !key && value) nlAfterValueInSeq = true - } - atLineStart = true - hasSpace = true - break - case 'anchor': - if (anchor) + else if (i < fc.items.length - 1) onError( - offset, - 'MULTIPLE_ANCHORS', - 'A node can have at most one anchor' + props.start, + 'UNEXPECTED_TOKEN', + `Unexpected empty item in ${fcName}` ) - anchor = token.source.substring(1) - atLineStart = false - atValueEnd = false - hasSpace = false - break - case 'tag': { - if (tagName) - onError(offset, 'MULTIPLE_TAGS', 'A node can have at most one tag') - const tn = ctx.directives.tagName(token.source, m => - onError(offset, 'TAG_RESOLVE_FAILED', m) - ) - if (tn) tagName = tn - atLineStart = false - atValueEnd = false - hasSpace = false - break + if (props.comment) { + if (coll.comment) coll.comment += '\n' + props.comment + else coll.comment = props.comment + } + continue } - case 'explicit-key-ind': - if (anchor || tagName) - onError( - offset, - 'PROP_BEFORE_SEP', - 'Anchors and tags must be after the ? indicator' - ) - atExplicitKey = true - atLineStart = false - atValueEnd = false - hasSpace = false - break - case 'map-value-ind': { - if (key) { - if (value) { - onError( - offset, - 'BLOCK_AS_IMPLICIT_KEY', - 'Missing {} around pair used as mapping key' - ) - const map = new YAMLMap(ctx.schema) - map.flow = true - map.items.push(new Pair(key, value)) - map.range = [key.range[0], value.range[1]] - key = map as YAMLMap.Parsed - value = null - } // else explicit key - } else if (value) { - if (ctx.options.strict) { - const slMsg = - 'Implicit keys of flow sequence pairs need to be on a single line' - if (nlAfterValueInSeq) - onError(offset, 'MULTILINE_IMPLICIT_KEY', slMsg) - else if (seqKeyToken) { - if (containsNewline(seqKeyToken)) - onError(offset, 'MULTILINE_IMPLICIT_KEY', slMsg) - if (seqKeyToken.offset < offset - 1024) + if (!isMap && ctx.options.strict && containsNewline(key)) + onError( + props.start, + 'MULTILINE_IMPLICIT_KEY', + 'Implicit keys of flow sequence pairs need to be on a single line' + ) + } + if (i === 0) { + if (props.comma) + onError( + props.comma.offset, + 'UNEXPECTED_TOKEN', + `Unexpected , in ${fcName}` + ) + } else if (!props.comma) + onError(props.start, 'MISSING_CHAR', `Missing , between ${fcName} items`) + + for (const token of [key, value]) + if (token && (token.type === 'block-map' || token.type === 'block-seq')) + onError( + token.offset, + 'BLOCK_IN_FLOW', + 'Block collections are not allowed within flow collections' + ) + + if (!isMap && !sep && !props.found) { + // item is a value in a seq + // → key & sep are empty, start does not include ? or : + const valueNode = value + ? composeNode(ctx, value, props, onError) + : composeEmptyNode(ctx, props.end, sep, null, props, onError) + ;(coll as YAMLSeq).items.push(valueNode) + offset = valueNode.range[1] + } else { + // item is a key+value pair + + // key value + const keyStart = props.end + const keyNode = key + ? composeNode(ctx, key, props, onError) + : composeEmptyNode(ctx, keyStart, start, null, props, onError) + + // value properties + const valueProps = resolveProps(sep || [], { + ctx, + flow: fcName, + indicator: 'map-value-ind', + offset: keyNode.range[1], + onError, + startOnNewline: false + }) + + if (valueProps.found) { + if (!isMap && !props.found && ctx.options.strict) { + if (sep) + for (const st of sep) { + if (st === valueProps.found) break + if (st.type === 'newline') { onError( - offset, - 'KEY_OVER_1024_CHARS', - 'The : indicator must be at most 1024 chars after the start of an implicit flow sequence key' + st.offset, + 'MULTILINE_IMPLICIT_KEY', + 'Implicit keys of flow sequence pairs need to be on a single line' ) - seqKeyToken = null + break + } } - } - key = value - value = null - } else { - key = composeEmptyNode(ctx, offset, fc.items, i, getProps(), onError) - } - if (comment) { - key.comment = comment - comment = '' + if (props.start < valueProps.found.offset - 1024) + onError( + valueProps.found.offset, + 'KEY_OVER_1024_CHARS', + 'The : indicator must be at most 1024 chars after the start of an implicit flow sequence key' + ) } - atExplicitKey = false - atValueEnd = false - hasSpace = false - break - } - case 'comma': - if (key || value || anchor || tagName || atExplicitKey) addItem(i) - else + } else if (value) { + if ('source' in value && value.source && value.source[0] === ':') onError( - offset, - 'UNEXPECTED_TOKEN', - `Unexpected , in flow ${isMap ? 'map' : 'sequence'}` + value.offset, + 'MISSING_CHAR', + `Missing space after : in ${fcName}` ) - key = null - value = null - atExplicitKey = false - atValueEnd = true - hasSpace = false - nlAfterValueInSeq = false - seqKeyToken = null - break - case 'block-map': - case 'block-seq': - onError( - offset, - 'BLOCK_IN_FLOW', - 'Block collections are not allowed within flow collections' - ) - // fallthrough - default: { - if (value) + else onError( - offset, + valueProps.start, 'MISSING_CHAR', - 'Missing , between flow collection items' + `Missing , or : between ${fcName} items` ) - if (!isMap && !key && !atExplicitKey) seqKeyToken = token - value = composeNode(ctx, token, getProps(), onError) - offset = value.range[1] - atLineStart = false - isSourceToken = false - atValueEnd = false - hasSpace = false } + + // value value + const valueNode = value + ? composeNode(ctx, value, valueProps, onError) + : valueProps.found + ? composeEmptyNode(ctx, valueProps.end, sep, null, valueProps, onError) + : null + + const pair = new Pair(keyNode, valueNode) + if (isMap) coll.items.push(pair) + else { + const map = new YAMLMap(ctx.schema) + map.flow = true + map.items.push(pair) + ;(coll as YAMLSeq).items.push(map) + } + offset = valueNode ? valueNode.range[1] : valueProps.end } - if (isSourceToken) offset += (token as SourceToken).source.length } - if (key || value || anchor || tagName || atExplicitKey) - addItem(fc.items.length) const expectedEnd = isMap ? '}' : ']' const [ce, ...ee] = fc.end if (!ce || ce.source !== expectedEnd) { - const cs = isMap ? 'map' : 'sequence' onError( - offset, + offset + 1, 'MISSING_CHAR', - `Expected flow ${cs} to end with ${expectedEnd}` + `Expected ${fcName} to end with ${expectedEnd}` ) } if (ce) offset += ce.source.length if (ee.length > 0) { const end = resolveEnd(ee, offset, ctx.options.strict, onError) - if (end.comment) coll.comment = comment + if (end.comment) { + if (coll.comment) coll.comment += '\n' + end.comment + else coll.comment = end.comment + } offset = end.offset } diff --git a/src/compose/resolve-props.ts b/src/compose/resolve-props.ts index 5eb34441..419c27b4 100644 --- a/src/compose/resolve-props.ts +++ b/src/compose/resolve-props.ts @@ -4,6 +4,7 @@ import type { ComposeErrorHandler } from './composer.js' export interface ResolvePropsArg { ctx: ComposeContext + flow?: string indicator: 'doc-start' | 'explicit-key-ind' | 'map-value-ind' | 'seq-item-ind' offset: number onError: ComposeErrorHandler @@ -12,7 +13,7 @@ export interface ResolvePropsArg { export function resolveProps( tokens: SourceToken[], - { ctx, indicator, offset, onError, startOnNewline }: ResolvePropsArg + { ctx, flow, indicator, offset, onError, startOnNewline }: ResolvePropsArg ) { let spaceBefore = false let atNewline = startOnNewline @@ -22,14 +23,21 @@ export function resolveProps( let hasNewline = false let anchor = '' let tagName = '' + let comma: SourceToken | null = null let found: SourceToken | null = null let start: number | null = null for (const token of tokens) { switch (token.type) { case 'space': - // At the doc level, tabs at line start may be parsed as leading - // white space rather than indentation. - if (atNewline && indicator !== 'doc-start' && token.source[0] === '\t') + // At the doc level, tabs at line start may be parsed + // as leading white space rather than indentation. + // In a flow collection, only the parser handles indent. + if ( + !flow && + atNewline && + indicator !== 'doc-start' && + token.source[0] === '\t' + ) onError( token.offset, 'TAB_AS_INDENT', @@ -87,10 +95,26 @@ export function resolveProps( } case indicator: // Could here handle preceding comments differently + if (anchor || tagName) + onError( + token.offset, + 'BAD_PROP_ORDER', + `Anchors and tags must be after the ${token.source} indicator` + ) found = token atNewline = false hasSpace = false break + case 'comma': + if (flow) { + if (comma) + onError(token.offset, 'UNEXPECTED_TOKEN', `Unexpected , in ${flow}`) + comma = token + atNewline = false + hasSpace = false + break + } + // else fallthrough default: onError( token.offset, @@ -104,6 +128,7 @@ export function resolveProps( const last = tokens[tokens.length - 1] const end = last ? last.offset + last.source.length : offset return { + comma, found, spaceBefore, comment, diff --git a/src/compose/util-contains-newline.ts b/src/compose/util-contains-newline.ts index cd0c26ad..32cd8d3d 100644 --- a/src/compose/util-contains-newline.ts +++ b/src/compose/util-contains-newline.ts @@ -7,20 +7,16 @@ export function containsNewline(key: Token | null | undefined) { case 'scalar': case 'double-quoted-scalar': case 'single-quoted-scalar': - return key.source.includes('\n') + if (key.source.includes('\n')) return true + if (key.end) + for (const st of key.end) if (st.type === 'newline') return true + return false case 'flow-collection': - for (const token of key.items) { - switch (token.type) { - case 'newline': - return true - case 'alias': - case 'scalar': - case 'double-quoted-scalar': - case 'single-quoted-scalar': - case 'flow-collection': - if (containsNewline(token)) return true - break - } + for (const it of key.items) { + for (const st of it.start) if (st.type === 'newline') return true + if (it.sep) + for (const st of it.sep) if (st.type === 'newline') return true + if (containsNewline(it.key) || containsNewline(it.value)) return true } return false default: diff --git a/src/errors.ts b/src/errors.ts index 5bc3a7d9..97d34960 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -5,6 +5,7 @@ export type ErrorCode = | 'BAD_DIRECTIVE' | 'BAD_DQ_ESCAPE' | 'BAD_INDENT' + | 'BAD_PROP_ORDER' | 'BAD_SCALAR_START' | 'BLOCK_AS_IMPLICIT_KEY' | 'BLOCK_IN_FLOW' @@ -17,7 +18,6 @@ export type ErrorCode = | 'MULTIPLE_ANCHORS' | 'MULTIPLE_DOCS' | 'MULTIPLE_TAGS' - | 'PROP_BEFORE_SEP' | 'TAB_AS_INDENT' | 'TAG_RESOLVE_FAILED' | 'UNEXPECTED_TOKEN' diff --git a/src/parse/parser.ts b/src/parse/parser.ts index e770dc18..3d41d98e 100644 --- a/src/parse/parser.ts +++ b/src/parse/parser.ts @@ -116,6 +116,24 @@ function getFirstKeyStartProps(prev: SourceToken[]) { return prev.splice(i, prev.length) } +function fixFlowSeqItems(fc: FlowCollection) { + if (fc.start.type === 'flow-seq-start') { + for (const it of fc.items) { + if ( + it.sep && + !it.value && + !includesToken(it.start, 'explicit-key-ind') && + !includesToken(it.sep, 'map-value-ind') + ) { + Array.prototype.push.apply(it.start, it.sep) + delete it.sep + if (it.key) it.value = it.key + delete it.key + } + } + } +} + /** * A YAML concrete syntax tree (CST) parser * @@ -309,6 +327,7 @@ export class Parser { // For these, parent indent is needed instead of own if (token.type === 'block-scalar' || token.type === 'flow-collection') token.indent = 'indent' in top ? top.indent : -1 + if (token.type === 'flow-collection') fixFlowSeqItems(token) switch (top.type) { case 'document': top.value = token @@ -337,9 +356,14 @@ export class Parser { else it.value = token break } - case 'flow-collection': - top.items.push(token) - break + case 'flow-collection': { + const it = top.items[top.items.length - 1] + if (!it || it.value) + top.items.push({ start: [], key: token, sep: [] }) + else if (it.sep) it.value = token + else Object.assign(it, { key: token, sep: [] }) + return + } /* istanbul ignore next should not happen */ default: this.pop() @@ -652,31 +676,48 @@ export class Parser { } private flowCollection(fc: FlowCollection) { + const it = fc.items[fc.items.length - 1] if (this.type === 'flow-error-end') { - let top + let top: Token | undefined do { this.pop() top = this.peek(1) } while (top && top.type === 'flow-collection') } else if (fc.end.length === 0) { switch (this.type) { - case 'space': - case 'comment': - case 'newline': case 'comma': case 'explicit-key-ind': + if (!it || it.sep) fc.items.push({ start: [this.sourceToken] }) + else it.start.push(this.sourceToken) + return + case 'map-value-ind': + if (!it || it.value) + fc.items.push({ start: [], key: null, sep: [this.sourceToken] }) + else if (it.sep) it.sep.push(this.sourceToken) + else Object.assign(it, { key: null, sep: [this.sourceToken] }) + return + + case 'space': + case 'comment': + case 'newline': case 'anchor': case 'tag': - fc.items.push(this.sourceToken) + if (!it || it.value) fc.items.push({ start: [this.sourceToken] }) + else if (it.sep) it.sep.push(this.sourceToken) + else it.start.push(this.sourceToken) return case 'alias': case 'scalar': case 'single-quoted-scalar': - case 'double-quoted-scalar': - fc.items.push(this.flowScalar(this.type)) + case 'double-quoted-scalar': { + const fs = this.flowScalar(this.type) + if (!it || it.value) fc.items.push({ start: [], key: fs, sep: [] }) + else if (it.sep) this.stack.push(fs) + else Object.assign(it, { key: fs, sep: [] }) return + } case 'flow-map-end': case 'flow-seq-end': @@ -706,6 +747,7 @@ export class Parser { ) { const prev = getPrevProps(parent) const start = getFirstKeyStartProps(prev) + fixFlowSeqItems(fc) const sep = fc.end.splice(1, fc.end.length) sep.push(this.sourceToken) const map: BlockMap = { diff --git a/src/parse/tokens.ts b/src/parse/tokens.ts index 2703f787..8ee19939 100644 --- a/src/parse/tokens.ts +++ b/src/parse/tokens.ts @@ -87,7 +87,12 @@ export interface BlockSequence { type: 'block-seq' offset: number indent: number - items: Array<{ start: SourceToken[]; value?: Token; sep?: never }> + items: Array<{ + start: SourceToken[] + key?: never + sep?: never + value?: Token + }> } export interface FlowCollection { @@ -95,7 +100,12 @@ export interface FlowCollection { offset: number indent: number start: SourceToken - items: Array + items: Array<{ + start: SourceToken[] + key?: Token | null + sep?: SourceToken[] + value?: Token + }> end: SourceToken[] } diff --git a/tests/doc/errors.js b/tests/doc/errors.js index 16f01acd..76db875e 100644 --- a/tests/doc/errors.js +++ b/tests/doc/errors.js @@ -122,24 +122,17 @@ describe('block collections', () => { describe('flow collections', () => { test('start only of flow map (eemeli/yaml#8)', () => { const doc = YAML.parseDocument('{') - const message = expect.stringContaining('Expected flow map to end with }') - expect(doc.errors).toMatchObject([{ message, offset: 1 }]) + expect(doc.errors).toMatchObject([{ code: 'MISSING_CHAR', offset: 1 }]) }) test('start only of flow sequence (eemeli/yaml#8)', () => { const doc = YAML.parseDocument('[') - const message = expect.stringContaining( - 'Expected flow sequence to end with ]' - ) - expect(doc.errors).toMatchObject([{ message, offset: 1 }]) + expect(doc.errors).toMatchObject([{ code: 'MISSING_CHAR', offset: 1 }]) }) test('flow sequence without end', () => { const doc = YAML.parseDocument('[ foo, bar,') - const message = expect.stringContaining( - 'Expected flow sequence to end with ]' - ) - expect(doc.errors).toMatchObject([{ message, offset: 11 }]) + expect(doc.errors).toMatchObject([{ code: 'MISSING_CHAR', offset: 11 }]) }) test('doc-end within flow sequence', () => { @@ -147,7 +140,7 @@ describe('flow collections', () => { prettyErrors: false }) expect(doc.errors).toMatchObject([ - { message: 'Expected flow sequence to end with ]' }, + { code: 'MISSING_CHAR' }, { message: 'Unexpected flow-seq-end token in YAML document: "]"' }, { message: @@ -166,18 +159,17 @@ describe('flow collections', () => { test('block seq in flow collection', () => { const doc = YAML.parseDocument('{\n- foo\n}') - expect(doc.errors).toHaveLength(1) - expect(doc.errors[0].message).toMatch( - 'Block collections are not allowed within flow collections' - ) + expect(doc.errors).toMatchObject([{ code: 'BLOCK_IN_FLOW' }]) }) - test('anchor before explicit key indicator', () => { + test.skip('anchor before explicit key indicator in block map', () => { + const doc = YAML.parseDocument('&a ? A') + expect(doc.errors).toMatchObject([{ code: 'BAD_PROP_ORDER' }]) + }) + + test('anchor before explicit key indicator in flow map', () => { const doc = YAML.parseDocument('{ &a ? A }') - expect(doc.errors).toHaveLength(1) - expect(doc.errors[0].message).toMatch( - 'Anchors and tags must be after the ? indicator' - ) + expect(doc.errors).toMatchObject([{ code: 'BAD_PROP_ORDER' }]) }) }) @@ -228,12 +220,14 @@ describe('pretty errors', () => { expect(docs[0].errors[0]).not.toHaveProperty('source') expect(docs[1].errors).toMatchObject([ { + code: 'UNEXPECTED_TOKEN', message: 'Unexpected , in flow map at line 3, column 7:\n\n{ 123,,, }\n ^\n', offset: 16, linePos: { line: 3, col: 7 } }, { + code: 'UNEXPECTED_TOKEN', message: 'Unexpected , in flow map at line 3, column 8:\n\n{ 123,,, }\n ^\n', offset: 17, diff --git a/tests/doc/parse.js b/tests/doc/parse.js index 2e7e468f..c07c0601 100644 --- a/tests/doc/parse.js +++ b/tests/doc/parse.js @@ -446,9 +446,9 @@ test('comment between key & : in flow collection (eemeli/yaml#149)', () => { expect(YAML.parse(src1)).toEqual({ a: 1 }) const src2 = '{a\n#c\n:1}' - expect(() => YAML.parse(src2)).toThrow( - 'Missing , between flow collection items' - ) + expect(async () => YAML.parse(src2)).rejects.toMatchObject({ + code: 'MISSING_CHAR' + }) }) describe('empty(ish) nodes', () => { From e20f5da744d5ad4ab26356e945839396fbbe5c87 Mon Sep 17 00:00:00 2001 From: Eemeli Aro Date: Sun, 28 Mar 2021 19:57:56 +0300 Subject: [PATCH 2/5] fix: Properly complain about `&a ? b` --- src/parse/parser.ts | 8 ++++++-- tests/doc/errors.js | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/parse/parser.ts b/src/parse/parser.ts index 3d41d98e..48234140 100644 --- a/src/parse/parser.ts +++ b/src/parse/parser.ts @@ -813,14 +813,18 @@ export class Parser { indent: this.indent, items: [{ start: [this.sourceToken] }] } as BlockSequence - case 'explicit-key-ind': + case 'explicit-key-ind': { this.onKeyLine = true + const prev = getPrevProps(parent) + const start = getFirstKeyStartProps(prev) + start.push(this.sourceToken) return { type: 'block-map', offset: this.offset, indent: this.indent, - items: [{ start: [this.sourceToken] }] + items: [{ start }] } as BlockMap + } case 'map-value-ind': { this.onKeyLine = true const prev = getPrevProps(parent) diff --git a/tests/doc/errors.js b/tests/doc/errors.js index 76db875e..2d397aff 100644 --- a/tests/doc/errors.js +++ b/tests/doc/errors.js @@ -162,7 +162,7 @@ describe('flow collections', () => { expect(doc.errors).toMatchObject([{ code: 'BLOCK_IN_FLOW' }]) }) - test.skip('anchor before explicit key indicator in block map', () => { + test('anchor before explicit key indicator in block map', () => { const doc = YAML.parseDocument('&a ? A') expect(doc.errors).toMatchObject([{ code: 'BAD_PROP_ORDER' }]) }) From 9340ef7812f0bcf43f5dbf270c49a2f1f91e6379 Mon Sep 17 00:00:00 2001 From: Eemeli Aro Date: Sun, 28 Mar 2021 21:08:03 +0300 Subject: [PATCH 3/5] fix: Flow comment parsing --- src/compose/resolve-flow-collection.ts | 43 ++++++++++++++++++-- src/parse/parser.ts | 9 +++-- tests/doc/comments.js | 54 ++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 7 deletions(-) diff --git a/src/compose/resolve-flow-collection.ts b/src/compose/resolve-flow-collection.ts index ed248ef6..aadaa299 100644 --- a/src/compose/resolve-flow-collection.ts +++ b/src/compose/resolve-flow-collection.ts @@ -1,3 +1,4 @@ +import { isPair } from '../nodes/Node.js' import { Pair } from '../nodes/Pair.js' import { YAMLMap } from '../nodes/YAMLMap.js' import { YAMLSeq } from '../nodes/YAMLSeq.js' @@ -16,7 +17,9 @@ export function resolveFlowCollection( ) { const isMap = fc.start.source === '{' const fcName = isMap ? 'flow map' : 'flow sequence' - const coll = isMap ? new YAMLMap(ctx.schema) : new YAMLSeq(ctx.schema) + const coll = isMap + ? (new YAMLMap(ctx.schema) as YAMLMap.Parsed) + : (new YAMLSeq(ctx.schema) as YAMLSeq.Parsed) coll.flow = true let offset = fc.offset @@ -64,8 +67,36 @@ export function resolveFlowCollection( 'UNEXPECTED_TOKEN', `Unexpected , in ${fcName}` ) - } else if (!props.comma) - onError(props.start, 'MISSING_CHAR', `Missing , between ${fcName} items`) + } else { + if (!props.comma) + onError( + props.start, + 'MISSING_CHAR', + `Missing , between ${fcName} items` + ) + if (props.comment) { + let prevItemComment = '' + loop: for (const st of start) { + switch (st.type) { + case 'comma': + case 'space': + break + case 'comment': + prevItemComment = st.source.substring(1) + break loop + default: + break loop + } + } + if (prevItemComment) { + let prev = coll.items[coll.items.length - 1] + if (isPair(prev)) prev = prev.value || prev.key + if (prev.comment) prev.comment += '\n' + prevItemComment + else prev.comment = prevItemComment + props.comment = props.comment.substring(prevItemComment.length + 1) + } + } + } for (const token of [key, value]) if (token && (token.type === 'block-map' || token.type === 'block-seq')) @@ -144,9 +175,13 @@ export function resolveFlowCollection( : valueProps.found ? composeEmptyNode(ctx, valueProps.end, sep, null, valueProps, onError) : null + if (!valueNode && valueProps.comment) { + if (keyNode.comment) keyNode.comment += '\n' + valueProps.comment + else keyNode.comment = valueProps.comment + } const pair = new Pair(keyNode, valueNode) - if (isMap) coll.items.push(pair) + if (isMap) (coll as YAMLMap.Parsed).items.push(pair) else { const map = new YAMLMap(ctx.schema) map.flow = true diff --git a/src/parse/parser.ts b/src/parse/parser.ts index 48234140..b74220e3 100644 --- a/src/parse/parser.ts +++ b/src/parse/parser.ts @@ -66,7 +66,7 @@ function atFirstEmptyLineAfterComments(start: SourceToken[]) { } function isFlowToken( - token: Token | null + token: Token | null | undefined ): token is FlowScalar | FlowCollection { switch (token?.type) { case 'alias': @@ -125,10 +125,13 @@ function fixFlowSeqItems(fc: FlowCollection) { !includesToken(it.start, 'explicit-key-ind') && !includesToken(it.sep, 'map-value-ind') ) { - Array.prototype.push.apply(it.start, it.sep) - delete it.sep if (it.key) it.value = it.key delete it.key + if (isFlowToken(it.value)) { + if (it.value.end) Array.prototype.push.apply(it.value.end, it.sep) + else it.value.end = it.sep + } else Array.prototype.push.apply(it.start, it.sep) + delete it.sep } } } diff --git a/tests/doc/comments.js b/tests/doc/comments.js index 05bbfb94..8ef642ee 100644 --- a/tests/doc/comments.js +++ b/tests/doc/comments.js @@ -261,6 +261,30 @@ describe('parse comments', () => { }) describe('flow collection commens', () => { + test('line comment after , in seq', () => { + const doc = YAML.parseDocument(source` + [ a, #c0 + b #c1 + ]`) + expect(doc.contents.items).toMatchObject([ + { value: 'a', comment: 'c0' }, + { value: 'b', comment: 'c1' } + ]) + }) + + test('line comment after , in map', () => { + const doc = YAML.parseDocument(source` + { a, #c0 + b: c, #c1 + d #c2 + }`) + expect(doc.contents.items).toMatchObject([ + { key: { value: 'a', comment: 'c0' } }, + { key: { value: 'b' }, value: { value: 'c', comment: 'c1' } }, + { key: { value: 'd', comment: 'c2' } } + ]) + }) + test('multi-line comments', () => { const doc = YAML.parseDocument('{ a,\n#c0\n#c1\nb }') expect(doc.contents.items).toMatchObject([ @@ -446,6 +470,36 @@ describe('stringify comments', () => { `) }) }) + + describe.skip('flow collection commens', () => { + test('line comment after , in seq', () => { + const doc = YAML.parseDocument(source` + [ a, #c0 + b #c1 + ]`) + expect(String(doc)).toBe(source` + [ + a, #c0 + b #c1 + ] + `) + }) + + test('line comment after , in map', () => { + const doc = YAML.parseDocument(source` + { a, #c0 + b: c, #c1 + d #c2 + }`) + expect(String(doc)).toBe(source` + { + ? a, #c0 + b: c, #c1 + ? d #c2 + } + `) + }) + }) }) describe('blank lines', () => { From 2ead77d05d7104a7aee4b22df0779998069e8f23 Mon Sep 17 00:00:00 2001 From: Eemeli Aro Date: Mon, 29 Mar 2021 00:21:26 +0300 Subject: [PATCH 4/5] feat!: Make Pair not extend NodeBase; drop its prop forwarding BREAKING CHANGE: While the inheritance of the class is just an implementation detail, dropping the prop forwarding means that a user that previously set the `commentBefore`, `comment` or `spaceBefore` property of a pair will now need to set it for the appropriate key or value. While adjusting the test suite for the changes, the somewhat surprising behaviour of multi-line comments on scalar values getting handled as `commentBefore` instead of `comment` is also dropped. --- docs/05_content_nodes.md | 2 +- src/compose/composer.ts | 5 ++- src/nodes/Pair.ts | 38 +++------------- src/schema/types.ts | 2 +- src/schema/yaml-1.1/pairs.ts | 26 ++++++----- src/stringify/stringifyCollection.ts | 40 +++++++++++------ src/stringify/stringifyPair.ts | 23 +++------- src/stringify/stringifyString.ts | 14 +----- src/test-events.ts | 2 +- tests/doc/comments.js | 67 ++++++++-------------------- tests/doc/parse.js | 2 +- 11 files changed, 82 insertions(+), 139 deletions(-) diff --git a/docs/05_content_nodes.md b/docs/05_content_nodes.md index afc9b5a2..9e7a89f6 100644 --- a/docs/05_content_nodes.md +++ b/docs/05_content_nodes.md @@ -46,7 +46,7 @@ On the other hand, `!!int` and `!!float` stringifiers will take `format` into ac ## Collections ```js -class Pair extends NodeBase { +class Pair { key: K // When parsed, key and value are always value: V // Node or null, but can be set to anything } diff --git a/src/compose/composer.ts b/src/compose/composer.ts index 0749d8f4..2a1013d5 100644 --- a/src/compose/composer.ts +++ b/src/compose/composer.ts @@ -1,7 +1,7 @@ import { Directives } from '../doc/directives.js' import { Document } from '../doc/Document.js' import { ErrorCode, YAMLParseError, YAMLWarning } from '../errors.js' -import { isCollection } from '../nodes/Node.js' +import { isCollection, isPair } from '../nodes/Node.js' import { defaultOptions, DocumentOptions, @@ -99,7 +99,8 @@ export class Composer { } else if (afterEmptyLine || doc.directives.marker || !dc) { doc.commentBefore = comment } else if (isCollection(dc) && !dc.flow && dc.items.length > 0) { - const it = dc.items[0] + let it = dc.items[0] + if (isPair(it)) it = it.key const cb = it.commentBefore it.commentBefore = cb ? `${comment}\n${cb}` : comment } else { diff --git a/src/nodes/Pair.ts b/src/nodes/Pair.ts index 02542109..e751cc2e 100644 --- a/src/nodes/Pair.ts +++ b/src/nodes/Pair.ts @@ -2,8 +2,7 @@ import { createNode, CreateNodeContext } from '../doc/createNode.js' import { StringifyContext } from '../stringify/stringify.js' import { stringifyPair } from '../stringify/stringifyPair.js' import { addPairToJSMap } from './addPairToJSMap.js' -import { isNode, NodeBase, PAIR } from './Node.js' -import { Scalar } from './Scalar.js' +import { NODE_TYPE, PAIR } from './Node.js' import { ToJSContext } from './toJS.js' export function createPair( @@ -16,7 +15,9 @@ export function createPair( return new Pair(k, v) } -export class Pair extends NodeBase { +export class Pair { + readonly [NODE_TYPE]: symbol + /** Always Node or null when parsed, but can be set to anything. */ key: K @@ -24,38 +25,9 @@ export class Pair extends NodeBase { value: V | null constructor(key: K, value: V | null = null) { - super(PAIR) + Object.defineProperty(this, NODE_TYPE, { value: PAIR }) this.key = key this.value = value - - // TS doesn't allow for accessors to override properties - // https://github.com/microsoft/TypeScript/pull/33509 - Object.defineProperties(this, { - commentBefore: { - get: () => (isNode(this.key) ? this.key.commentBefore : undefined), - set: (cb: string | null) => { - if (this.key == null) this.key = (new Scalar(null) as unknown) as K - if (isNode(this.key)) this.key.commentBefore = cb - else { - const msg = - 'Pair.commentBefore is an alias for Pair.key.commentBefore. To set it, the key must be a Node.' - throw new Error(msg) - } - } - }, - spaceBefore: { - get: () => (isNode(this.key) ? this.key.spaceBefore : undefined), - set: (sb: boolean) => { - if (this.key == null) this.key = (new Scalar(null) as unknown) as K - if (isNode(this.key)) this.key.spaceBefore = sb - else { - const msg = - 'Pair.spaceBefore is an alias for Pair.key.spaceBefore. To set it, the key must be a Node.' - throw new Error(msg) - } - } - } - }) } toJSON(_?: unknown, ctx?: ToJSContext): ReturnType { diff --git a/src/schema/types.ts b/src/schema/types.ts index c580d9ad..9ee2368a 100644 --- a/src/schema/types.ts +++ b/src/schema/types.ts @@ -100,7 +100,7 @@ export interface CollectionTag extends TagBase { * If returning a non-`Node` value, the output will be wrapped as a `Scalar`. */ resolve( - value: YAMLMap | YAMLSeq, + value: YAMLMap.Parsed | YAMLSeq.Parsed, onError: (message: string) => void, options: ParseOptions ): unknown diff --git a/src/schema/yaml-1.1/pairs.ts b/src/schema/yaml-1.1/pairs.ts index 9cbf039a..6a89cb1f 100644 --- a/src/schema/yaml-1.1/pairs.ts +++ b/src/schema/yaml-1.1/pairs.ts @@ -1,13 +1,16 @@ import type { CreateNodeContext } from '../../doc/createNode.js' -import type { Schema } from '../../schema/Schema.js' -import { isMap, isPair, isSeq } from '../../nodes/Node.js' +import { isMap, isPair, isSeq, ParsedNode } from '../../nodes/Node.js' import { createPair, Pair } from '../../nodes/Pair.js' +import { Scalar } from '../../nodes/Scalar.js' import { YAMLMap } from '../../nodes/YAMLMap.js' import { YAMLSeq } from '../../nodes/YAMLSeq.js' +import type { Schema } from '../../schema/Schema.js' import type { CollectionTag } from '../types.js' export function resolvePairs( - seq: YAMLSeq | YAMLMap, + seq: + | YAMLSeq.Parsed> + | YAMLMap.Parsed, onError: (message: string) => void ) { if (isSeq(seq)) { @@ -17,21 +20,24 @@ export function resolvePairs( else if (isMap(item)) { if (item.items.length > 1) onError('Each pair must have its own sequence indicator') - const pair = item.items[0] || new Pair(null) + const pair = + item.items[0] || new Pair(new Scalar(null) as Scalar.Parsed) if (item.commentBefore) - pair.commentBefore = pair.commentBefore - ? `${item.commentBefore}\n${pair.commentBefore}` + pair.key.commentBefore = pair.key.commentBefore + ? `${item.commentBefore}\n${pair.key.commentBefore}` : item.commentBefore - if (item.comment) - pair.comment = pair.comment - ? `${item.comment}\n${pair.comment}` + if (item.comment) { + const cn = pair.value || pair.key + cn.comment = cn.comment + ? `${item.comment}\n${cn.comment}` : item.comment + } item = pair } seq.items[i] = isPair(item) ? item : new Pair(item) } } else onError('Expected a sequence for this tag') - return seq as YAMLSeq + return seq as YAMLSeq.Parsed> } export function createPairs( diff --git a/src/stringify/stringifyCollection.ts b/src/stringify/stringifyCollection.ts index 09fc00eb..111feb20 100644 --- a/src/stringify/stringifyCollection.ts +++ b/src/stringify/stringifyCollection.ts @@ -28,29 +28,43 @@ export function stringifyCollection( const inFlow = flow || ctx.inFlow if (inFlow) itemIndent += indentStep ctx = Object.assign({}, ctx, { indent: itemIndent, inFlow, type: null }) - let chompKeep = false + let chompKeep = false // flag for the preceding node's status let hasItemWithNewLine = false const nodes = items.reduce((nodes: StringifyNode[], item, i) => { let comment: string | null = null - if (isNode(item) || isPair(item)) { - if (!chompKeep && item.spaceBefore) nodes.push({ comment: true, str: '' }) - + if (isNode(item)) { + if (!chompKeep && item.spaceBefore) { + nodes.push({ comment: true, str: '' }) + if (inFlow) hasItemWithNewLine = true + } if (item.commentBefore) { // This match will always succeed on a non-empty string for (const line of item.commentBefore.match(/^.*$/gm) as string[]) nodes.push({ comment: true, str: `#${line}` }) + if (inFlow) hasItemWithNewLine = true + } + if (item.comment) { + comment = item.comment + if (inFlow) hasItemWithNewLine = true + } + } else if (isPair(item)) { + if (isNode(item.key)) { + if (!chompKeep && item.key.spaceBefore) { + nodes.push({ comment: true, str: '' }) + if (inFlow) hasItemWithNewLine = true + } + if (item.key.commentBefore) { + // This match will always succeed on a non-empty string + for (const line of item.key.commentBefore.match(/^.*$/gm) as string[]) + nodes.push({ comment: true, str: `#${line}` }) + if (inFlow) hasItemWithNewLine = true + } + if (inFlow && item.key.comment) hasItemWithNewLine = true } - - if (item.comment) comment = item.comment - - const pair = item as any // Apply guards manually in the following if ( inFlow && - ((!chompKeep && item.spaceBefore) || - item.commentBefore || - item.comment || - (pair.key && (pair.key.commentBefore || pair.key.comment)) || - (pair.value && (pair.value.commentBefore || pair.value.comment))) + isNode(item.value) && + (item.value.commentBefore || item.value.comment) ) hasItemWithNewLine = true } diff --git a/src/stringify/stringifyPair.ts b/src/stringify/stringifyPair.ts index 49b14a4e..cee3303b 100644 --- a/src/stringify/stringifyPair.ts +++ b/src/stringify/stringifyPair.ts @@ -5,9 +5,9 @@ import { addComment } from './addComment.js' import { stringify, StringifyContext } from './stringify.js' export function stringifyPair( - { comment, key, value }: Readonly, + { key, value }: Readonly, ctx: StringifyContext, - onComment?: () => void, + _onComment?: () => void, onChompKeep?: () => void ) { const { @@ -30,7 +30,7 @@ export function stringifyPair( let explicitKey = !simpleKeys && (!key || - (keyComment && value == null) || + (keyComment && value == null && !ctx.inFlow) || isCollection(key) || (isScalar(key) ? key.type === Scalar.BLOCK_FOLDED || key.type === Scalar.BLOCK_LITERAL @@ -62,24 +62,13 @@ export function stringifyPair( (value == null && (explicitKey || ctx.inFlow)) ) { str = addComment(str, ctx.indent, keyComment) - if (comment) { - if (keyComment && !comment.includes('\n')) - str += `\n${ctx.indent || ''}#${comment}` - else str = addComment(str, ctx.indent, comment) - if (onComment) onComment() - } else if (chompKeep && !keyComment && onChompKeep) onChompKeep() + if (chompKeep && !keyComment && onChompKeep) onChompKeep() return ctx.inFlow && !explicitKey ? str : `? ${str}` } str = explicitKey ? `? ${addComment(str, ctx.indent, keyComment)}\n${indent}:` : addComment(`${str}:`, ctx.indent, keyComment) - if (comment) { - if (keyComment && !explicitKey && !comment.includes('\n')) - str += `\n${ctx.indent || ''}#${comment}` - else str = addComment(str, ctx.indent, comment) - if (onComment) onComment() - } let vcb = '' let valueComment = null @@ -94,7 +83,7 @@ export function stringifyPair( value = doc.createNode(value) } ctx.implicitKey = false - if (!explicitKey && !keyComment && !comment && isScalar(value)) + if (!explicitKey && !keyComment && isScalar(value)) ctx.indentAtStart = str.length + 1 chompKeep = false if ( @@ -117,7 +106,7 @@ export function stringifyPair( () => (chompKeep = true) ) let ws = ' ' - if (vcb || keyComment || comment) { + if (vcb || keyComment) { ws = `${vcb}\n${ctx.indent}` } else if (!explicitKey && isCollection(value)) { const flow = valueStr[0] === '[' || valueStr[0] === '{' diff --git a/src/stringify/stringifyString.ts b/src/stringify/stringifyString.ts index 3b2f088b..98da1437 100644 --- a/src/stringify/stringifyString.ts +++ b/src/stringify/stringifyString.ts @@ -1,5 +1,4 @@ import { Scalar } from '../nodes/Scalar.js' -import { addCommentBefore } from './addComment.js' import { foldFlowLines, FoldOptions, @@ -227,7 +226,7 @@ function plainString( onComment?: () => void, onChompKeep?: () => void ) { - const { comment, type, value } = item + const { type, value } = item const { actualString, implicitKey, indent, inFlow } = ctx if ( (implicitKey && /[\n[\]{},]/.test(value)) || @@ -290,18 +289,9 @@ function plainString( return doubleQuotedString(value, ctx) } } - const body = implicitKey + return implicitKey ? str : foldFlowLines(str, indent, FOLD_FLOW, getFoldOptions(ctx)) - if ( - comment && - !inFlow && - (body.indexOf('\n') !== -1 || comment.indexOf('\n') !== -1) - ) { - if (onComment) onComment() - return addCommentBefore(body, indent, comment) - } - return body } export function stringifyString( diff --git a/src/test-events.ts b/src/test-events.ts index 46a7b547..4da8d7d0 100644 --- a/src/test-events.ts +++ b/src/test-events.ts @@ -93,7 +93,7 @@ function addEvents( } props = ` &${anchor}` } - if (node.tag) props += ` <${node.tag}>` + if (isNode(node) && node.tag) props += ` <${node.tag}>` if (isMap(node)) { events.push(`+MAP${props}`) diff --git a/tests/doc/comments.js b/tests/doc/comments.js index 8ef642ee..e24afcd1 100644 --- a/tests/doc/comments.js +++ b/tests/doc/comments.js @@ -189,8 +189,8 @@ describe('parse comments', () => { commentBefore: 'c0\nc1', items: [ {}, - { commentBefore: 'c2', value: { comment: 'c3' } }, - { commentBefore: 'c4' } + { key: { commentBefore: 'c2' }, value: { comment: 'c3' } }, + { key: { commentBefore: 'c4' } } ] } ] @@ -289,7 +289,7 @@ describe('parse comments', () => { const doc = YAML.parseDocument('{ a,\n#c0\n#c1\nb }') expect(doc.contents.items).toMatchObject([ { key: { value: 'a' } }, - { commentBefore: 'c0\nc1', key: { value: 'b' } } + { key: { commentBefore: 'c0\nc1', value: 'b' } } ]) }) }) @@ -324,7 +324,7 @@ describe('stringify comments', () => { const src = 'string' const doc = YAML.parseDocument(src) doc.contents.comment = 'comment\nlines' - expect(String(doc)).toBe('#comment\n#lines\nstring\n') + expect(String(doc)).toBe('string\n#comment\n#lines\n') }) test('"quoted"', () => { @@ -404,8 +404,8 @@ describe('stringify comments', () => { const doc = YAML.parseDocument(src) doc.contents.items[0].key.commentBefore = 'c0' doc.contents.items[0].key.comment = 'c1' - doc.contents.items[0].comment = 'c2' const seq = doc.contents.items[0].value + seq.commentBefore = 'c2' seq.items[0].commentBefore = 'c3' seq.items[1].commentBefore = 'c4' seq.comment = 'c5' @@ -426,9 +426,9 @@ describe('stringify comments', () => { test('plain', () => { const src = 'key1: value 1\nkey2: value 2\n' const doc = YAML.parseDocument(src) - doc.contents.items[0].commentBefore = 'c0' - doc.contents.items[1].commentBefore = 'c1' - doc.contents.items[1].comment = 'c2' + doc.contents.items[0].key.commentBefore = 'c0' + doc.contents.items[1].key.commentBefore = 'c1' + doc.contents.items[1].key.comment = 'c2' doc.contents.items[1].value.spaceBefore = true doc.contents.comment = 'c3' expect(String(doc)).toBe(source` @@ -445,9 +445,9 @@ describe('stringify comments', () => { test('multiline', () => { const src = 'key1: value 1\nkey2: value 2\n' const doc = YAML.parseDocument(src) - doc.contents.items[0].commentBefore = 'c0\nc1' - doc.contents.items[1].commentBefore = '\nc2\n\nc3' - doc.contents.items[1].comment = 'c4\nc5' + doc.contents.items[0].key.commentBefore = 'c0\nc1' + doc.contents.items[1].key.commentBefore = '\nc2\n\nc3' + doc.contents.items[1].key.comment = 'c4\nc5' doc.contents.items[1].value.spaceBefore = true doc.contents.items[1].value.commentBefore = 'c6' doc.contents.comment = 'c7\nc8' @@ -471,7 +471,7 @@ describe('stringify comments', () => { }) }) - describe.skip('flow collection commens', () => { + describe('flow collection commens', () => { test('line comment after , in seq', () => { const doc = YAML.parseDocument(source` [ a, #c0 @@ -485,7 +485,7 @@ describe('stringify comments', () => { `) }) - test('line comment after , in map', () => { + test.skip('line comment after , in map', () => { const doc = YAML.parseDocument(source` { a, #c0 b: c, #c1 @@ -595,8 +595,10 @@ describe('blank lines', () => { test(name, () => { const doc = YAML.parseDocument(src) expect(String(doc)).toBe(src) - expect(doc.contents.items[1]).not.toHaveProperty('spaceBefore', true) - doc.contents.items[1].spaceBefore = true + let it = doc.contents.items[1] + if (it.key) it = it.key + expect(it).not.toHaveProperty('spaceBefore', true) + it.spaceBefore = true expect(String(doc)).toBe(src) }) } @@ -666,8 +668,8 @@ describe('blank lines', () => { const doc = YAML.parseDocument(src) expect(doc.contents).toMatchObject({ items: [ - { key: { value: 'a' }, value: { value: 1 }, spaceBefore: true }, - { key: { value: 'b' }, value: { value: 2 }, spaceBefore: true } + { key: { value: 'a', spaceBefore: true }, value: { value: 1 } }, + { key: { value: 'b', spaceBefore: true }, value: { value: 2 } } ] }) }) @@ -918,34 +920,3 @@ a: expect(String(doc)).toBe(src) }) }) - -describe('Pair.commentBefore', () => { - test('Should get key comment', () => { - const key = new YAML.Document().createNode('foo') - const pair = new YAML.Pair(key, 42) - key.commentBefore = 'cc' - expect(pair.commentBefore).toBe('cc') - }) - - test('Should set key comment', () => { - const key = new YAML.Document().createNode('foo') - const pair = new YAML.Pair(key, 42) - pair.commentBefore = 'cc' - expect(key.commentBefore).toBe('cc') - }) - - test('Should create a key from a null value', () => { - const pair = new YAML.Pair(null, 42) - expect(pair.key).toBeNull() - pair.commentBefore = 'cc' - expect(pair.key).not.toBeNull() - expect(pair.key.commentBefore).toBe('cc') - }) - - test('Should throw for non-Node key', () => { - const pair = new YAML.Pair({ foo: 'bar' }, 42) - expect(() => { - pair.commentBefore = 'cc' - }).toThrow(/commentBefore is an alias/) - }) -}) diff --git a/tests/doc/parse.js b/tests/doc/parse.js index c07c0601..03adf423 100644 --- a/tests/doc/parse.js +++ b/tests/doc/parse.js @@ -489,7 +489,7 @@ describe('maps with no values', () => { const src = `{\na: null,\n? b\n}` const doc = YAML.parseDocument(src) expect(String(doc)).toBe(`{ a: null, b }\n`) - doc.contents.items[1].comment = 'c' + doc.contents.items[1].key.comment = 'c' expect(String(doc)).toBe(`{\n a: null,\n b #c\n}\n`) doc.set('b', 'x') expect(String(doc)).toBe(`{\n a: null,\n b: #c\n x\n}\n`) From bbfffae181bbc69685313e1bfcf1f096e33049fc Mon Sep 17 00:00:00 2001 From: Eemeli Aro Date: Mon, 29 Mar 2021 02:19:59 +0300 Subject: [PATCH 5/5] fix: Line comment handling in flow collections --- src/nodes/Collection.ts | 7 ++-- src/stringify/stringifyCollection.ts | 59 +++++++++++++++------------- src/stringify/stringifyPair.ts | 35 +++++++++++------ tests/doc/comments.js | 8 ++-- 4 files changed, 63 insertions(+), 46 deletions(-) diff --git a/src/nodes/Collection.ts b/src/nodes/Collection.ts index b40547e3..1c748987 100644 --- a/src/nodes/Collection.ts +++ b/src/nodes/Collection.ts @@ -1,7 +1,6 @@ import { createNode } from '../doc/createNode.js' import type { Schema } from '../schema/Schema.js' -import { isCollection, isNode, isScalar, NodeBase, NODE_TYPE } from './Node.js' -import type { Pair } from './Pair.js' +import { isCollection, isPair, isScalar, NodeBase, NODE_TYPE } from './Node.js' export function collectionFromPath( schema: Schema, @@ -143,8 +142,8 @@ export abstract class Collection extends NodeBase { hasAllNullValues(allowScalar?: boolean) { return this.items.every(node => { - if (!node || isNode(node)) return false - const n = (node as Pair).value + if (!isPair(node)) return false + const n = node.value return ( n == null || (allowScalar && diff --git a/src/stringify/stringifyCollection.ts b/src/stringify/stringifyCollection.ts index 111feb20..3f4b3b09 100644 --- a/src/stringify/stringifyCollection.ts +++ b/src/stringify/stringifyCollection.ts @@ -28,46 +28,45 @@ export function stringifyCollection( const inFlow = flow || ctx.inFlow if (inFlow) itemIndent += indentStep ctx = Object.assign({}, ctx, { indent: itemIndent, inFlow, type: null }) + + let singleLineOutput = true let chompKeep = false // flag for the preceding node's status - let hasItemWithNewLine = false const nodes = items.reduce((nodes: StringifyNode[], item, i) => { let comment: string | null = null if (isNode(item)) { - if (!chompKeep && item.spaceBefore) { - nodes.push({ comment: true, str: '' }) - if (inFlow) hasItemWithNewLine = true - } + if (!chompKeep && item.spaceBefore) nodes.push({ comment: true, str: '' }) if (item.commentBefore) { // This match will always succeed on a non-empty string for (const line of item.commentBefore.match(/^.*$/gm) as string[]) nodes.push({ comment: true, str: `#${line}` }) - if (inFlow) hasItemWithNewLine = true } if (item.comment) { comment = item.comment - if (inFlow) hasItemWithNewLine = true + singleLineOutput = false } } else if (isPair(item)) { - if (isNode(item.key)) { - if (!chompKeep && item.key.spaceBefore) { - nodes.push({ comment: true, str: '' }) - if (inFlow) hasItemWithNewLine = true - } - if (item.key.commentBefore) { + const ik = isNode(item.key) ? item.key : null + if (ik) { + if (!chompKeep && ik.spaceBefore) nodes.push({ comment: true, str: '' }) + if (ik.commentBefore) { // This match will always succeed on a non-empty string - for (const line of item.key.commentBefore.match(/^.*$/gm) as string[]) + for (const line of ik.commentBefore.match(/^.*$/gm) as string[]) nodes.push({ comment: true, str: `#${line}` }) - if (inFlow) hasItemWithNewLine = true } - if (inFlow && item.key.comment) hasItemWithNewLine = true + if (ik.comment) singleLineOutput = false + } + + if (inFlow) { + const iv = isNode(item.value) ? item.value : null + if (iv) { + if (iv.comment) comment = iv.comment + if (iv.comment || iv.commentBefore) singleLineOutput = false + } else if (item.value == null && ik && ik.comment) { + comment = ik.comment + } } - if ( - inFlow && - isNode(item.value) && - (item.value.commentBefore || item.value.comment) - ) - hasItemWithNewLine = true } + chompKeep = false let str = stringify( item, @@ -75,24 +74,30 @@ export function stringifyCollection( () => (comment = null), () => (chompKeep = true) ) - if (inFlow && !hasItemWithNewLine && str.includes('\n')) - hasItemWithNewLine = true if (inFlow && i < items.length - 1) str += ',' str = addComment(str, itemIndent, comment) if (chompKeep && (comment || inFlow)) chompKeep = false nodes.push({ comment: false, str }) return nodes }, []) + let str: string if (nodes.length === 0) { str = flowChars.start + flowChars.end } else if (inFlow) { const { start, end } = flowChars const strings = nodes.map(n => n.str) + let singleLineLength = 2 + for (const node of nodes) { + if (node.comment || node.str.includes('\n')) { + singleLineOutput = false + break + } + singleLineLength += node.str.length + 2 + } if ( - hasItemWithNewLine || - strings.reduce((sum, str) => sum + str.length + 2, 2) > - Collection.maxFlowStringSingleLineLength + !singleLineOutput || + singleLineLength > Collection.maxFlowStringSingleLineLength ) { str = start for (const s of strings) { diff --git a/src/stringify/stringifyPair.ts b/src/stringify/stringifyPair.ts index cee3303b..b522b24c 100644 --- a/src/stringify/stringifyPair.ts +++ b/src/stringify/stringifyPair.ts @@ -7,7 +7,7 @@ import { stringify, StringifyContext } from './stringify.js' export function stringifyPair( { key, value }: Readonly, ctx: StringifyContext, - _onComment?: () => void, + onComment?: () => void, onChompKeep?: () => void ) { const { @@ -41,11 +41,12 @@ export function stringifyPair( implicitKey: !explicitKey && (simpleKeys || !allNullValues), indent: indent + indentStep }) + let keyCommentDone = false let chompKeep = false let str = stringify( key, ctx, - () => (keyComment = null), + () => (keyCommentDone = true), () => (chompKeep = true) ) @@ -57,15 +58,18 @@ export function stringifyPair( explicitKey = true } - if ( - (allNullValues && (!simpleKeys || ctx.inFlow)) || - (value == null && (explicitKey || ctx.inFlow)) - ) { - str = addComment(str, ctx.indent, keyComment) + if (ctx.inFlow) { + if (allNullValues || value == null) { + if (keyCommentDone && onComment) onComment() + return explicitKey ? `? ${str}` : str + } + } else if ((allNullValues && !simpleKeys) || (value == null && explicitKey)) { + if (keyCommentDone) keyComment = null if (chompKeep && !keyComment && onChompKeep) onChompKeep() - return ctx.inFlow && !explicitKey ? str : `? ${str}` + return addComment(`? ${str}`, ctx.indent, keyComment) } + if (keyCommentDone) keyComment = null str = explicitKey ? `? ${addComment(str, ctx.indent, keyComment)}\n${indent}:` : addComment(`${str}:`, ctx.indent, keyComment) @@ -99,10 +103,12 @@ export function stringifyPair( // If indentSeq === false, consider '- ' as part of indentation where possible ctx.indent = ctx.indent.substr(2) } + + let valueCommentDone = false const valueStr = stringify( value, ctx, - () => (valueComment = null), + () => (valueCommentDone = true), () => (chompKeep = true) ) let ws = ' ' @@ -112,6 +118,13 @@ export function stringifyPair( const flow = valueStr[0] === '[' || valueStr[0] === '{' if (!flow || valueStr.includes('\n')) ws = `\n${ctx.indent}` } else if (valueStr[0] === '\n') ws = '' - if (chompKeep && !valueComment && onChompKeep) onChompKeep() - return addComment(str + ws + valueStr, ctx.indent, valueComment) + + if (ctx.inFlow) { + if (valueCommentDone && onComment) onComment() + return str + ws + valueStr + } else { + if (valueCommentDone) valueComment = null + if (chompKeep && !valueComment && onChompKeep) onChompKeep() + return addComment(str + ws + valueStr, ctx.indent, valueComment) + } } diff --git a/tests/doc/comments.js b/tests/doc/comments.js index e24afcd1..208d0e88 100644 --- a/tests/doc/comments.js +++ b/tests/doc/comments.js @@ -471,7 +471,7 @@ describe('stringify comments', () => { }) }) - describe('flow collection commens', () => { + describe('flow collection comments', () => { test('line comment after , in seq', () => { const doc = YAML.parseDocument(source` [ a, #c0 @@ -485,7 +485,7 @@ describe('stringify comments', () => { `) }) - test.skip('line comment after , in map', () => { + test('line comment after , in map', () => { const doc = YAML.parseDocument(source` { a, #c0 b: c, #c1 @@ -493,9 +493,9 @@ describe('stringify comments', () => { }`) expect(String(doc)).toBe(source` { - ? a, #c0 + a, #c0 b: c, #c1 - ? d #c2 + d #c2 } `) })