-
Notifications
You must be signed in to change notification settings - Fork 153
Preserve Inline comments #629
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,11 @@ export interface SourceToken { | |
| offset: number | ||
| indent: number | ||
| source: string | ||
| commentAfterKey?: boolean | ||
| commentOnParentKey?: string | ||
| parentComment?: string | ||
| commentIsAfterKey?: boolean | ||
| context?: any | ||
|
Comment on lines
+34
to
+38
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needing to add this many fields here is almost certainly a mistake. A SourceToken is meant to be a YAML syntax character, and not a vessel for carrying a lot of state.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree but I don't remember why I implemented this way now lol. I needed the references downstream and wasn't sure where the best place to do this would be |
||
| } | ||
|
|
||
| export interface ErrorToken { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,8 @@ import type { | |
| import { prettyToken, tokenType } from './cst.ts' | ||
| import { Lexer } from './lexer.ts' | ||
|
|
||
| const DEBUG = false | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is meant to be removed before merging, right?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah artifact of trying to debug the lib to implement this feature |
||
|
|
||
| function includesToken(list: SourceToken[], type: SourceToken['type']) { | ||
| for (let i = 0; i < list.length; ++i) if (list[i].type === type) return true | ||
| return false | ||
|
|
@@ -241,13 +243,81 @@ export class Parser { | |
| while (this.stack.length > 0) yield* this.pop() | ||
| } | ||
|
|
||
| private getCurrentContext(parent: Token) { | ||
| const top = parent | ||
| if (!top) return { context: 'stream' } | ||
|
|
||
| switch (top.type) { | ||
| case 'block-map': { | ||
| const it = top.items[top.items.length - 1] | ||
| if (!it) return { context: 'map-start' } | ||
| if (it.value) return { context: 'map-value', key: it.key } | ||
| if (it.sep) return { context: 'map-separator', sep: it.sep } | ||
| return { context: 'map-key', key: it.key } | ||
| } | ||
| case 'block-seq': { | ||
| const it = top.items[top.items.length - 1] | ||
| if (!it) return { context: 'seq-start' } | ||
| if (it.value) return { context: 'seq-value' } | ||
| return { context: 'seq-item' } | ||
| } | ||
| case 'flow-collection': { | ||
| const it = top.items[top.items.length - 1] | ||
| if (!it) return { context: 'flow-start' } | ||
| if (it.value) return { context: 'flow-value', key: it.key } | ||
| if (it.sep) return { context: 'flow-separator' } | ||
| return { context: 'flow-key', key: it.key } | ||
| } | ||
| default: | ||
| return { context: 'other', parentType: top.type } | ||
| } | ||
| } | ||
|
|
||
| private get sourceToken() { | ||
| const st: SourceToken = { | ||
| type: this.type as SourceToken['type'], | ||
| offset: this.offset, | ||
| indent: this.indent, | ||
| source: this.source | ||
| } | ||
|
|
||
| if (this.type === 'comment') { | ||
| const parent = this.peek(1) | ||
| const currentContext = this.getCurrentContext(parent) | ||
|
|
||
| if (DEBUG) { | ||
| st.context = currentContext | ||
| } | ||
|
|
||
| if (parent && 'items' in parent && parent.items && currentContext.context === 'map-separator') { | ||
| const it = parent.items[parent.items.length - 1] | ||
| if (it?.sep) { | ||
| if (DEBUG) st.parentComment = (it?.sep.find(st => st.type === 'comment') ?? {})?.source | ||
| // Check if this comment appears right after a map-value-ind token | ||
| const mapValueIndIndex = it.sep.findIndex(token => token.type === 'map-value-ind') | ||
| if (mapValueIndIndex !== -1) { | ||
| // Check if all tokens after map-value-ind are spaces (no newlines) followed by this comment | ||
| let allSpacesAfterMapValue = true | ||
| for (let i = mapValueIndIndex + 1; i < it.sep.length; i++) { | ||
| const token = it.sep[i] | ||
| if (token.type === 'newline') { | ||
| allSpacesAfterMapValue = false | ||
| break | ||
| } else if (token.type !== 'space') { | ||
| allSpacesAfterMapValue = false | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // If all tokens after map-value-ind are spaces (no newlines), this comment is commentIsAfterKey | ||
| if (allSpacesAfterMapValue) { | ||
| st.commentIsAfterKey = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return st | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -237,7 +237,6 @@ describe('parse comments', () => { | |
| { | ||
| key: { commentBefore: 'c0', value: 'k1' }, | ||
| value: { | ||
| commentBefore: 'c1', | ||
| items: [{ value: 'v1' }, { commentBefore: 'c2', value: 'v2' }], | ||
| comment: 'c3' | ||
| } | ||
|
|
@@ -252,8 +251,7 @@ describe('parse comments', () => { | |
| }) | ||
| expect(String(doc)).toBe(source` | ||
| #c0 | ||
| k1: | ||
| #c1 | ||
| k1: #c1 | ||
| - v1 | ||
| #c2 | ||
| - v2 | ||
|
|
@@ -1148,9 +1146,17 @@ entryB: | |
| }) | ||
|
|
||
| test('collection end comment', () => { | ||
| const src = `a: b #c\n#d\n` | ||
| const src = source` | ||
| a: b #c | ||
|
|
||
| #d | ||
| ` | ||
|
Comment on lines
-1151
to
+1153
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous test had a single newline before
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't remember 😅 Something about the trailing comment after all key value pairs is adding 2 newlines |
||
| const doc = YAML.parseDocument(src) | ||
| expect(String(doc)).toBe(`a: b #c\n\n#d\n`) | ||
| expect(String(doc)).toBe(source` | ||
| a: b #c | ||
|
|
||
| #d | ||
| `) | ||
| }) | ||
|
|
||
| test('blank line after seq in map', () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this new field really necessary? Couldn't the
commentvalue of the key be used instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc it was because there was conflict with comments before the line and on the key. Both were slotting into 'comment' and conflicting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments before the key ought to go into
commentBefore. There is a possibility of comments right after an explicit key but before the::but that's really quite rare.
As it happens, assigning a
commenton a key appears to already produce the desired output:results in
So perhaps the problem to fix here is that re-parsing the above result puts the comment as the
commentBeforeof the value, rather than attaching it as the keycomment?