Conversation
eemeli
left a comment
eemeli
left a comment
There was a problem hiding this comment.
Hey, sorry for not getting to this sooner -- I'd somehow managed to miss the notification about the PR.
I didn't look into the implementation details too much, as I have concerns about the new fields that are being proposed for Node and SourceToken.
My own initial sense is that in something like
key: # comment
valuewe ought to parse the comment as the .comment value of the key, rather than any new field. I'm also very doubtful that this really requires any new fields being added to the CST.
Then when serialising, whether or not the comment contains more than one line of content could be used to pick different behaviour.
Is there a reason why that approach wouldn't work?
| /** A comment after key: */ | ||
| declare commentAfterKey?: string | null | ||
|
|
There was a problem hiding this comment.
Is this new field really necessary? Couldn't the comment value of the key be used instead?
There was a problem hiding this comment.
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.
Comments before the key ought to go into commentBefore. There is a possibility of comments right after an explicit key but before the ::
? - key
# comment
: valuebut that's really quite rare.
As it happens, assigning a comment on a key appears to already produce the desired output:
let doc = parseDocument('key: value')
doc.contents.items[0].key.comment = 'ccc'
doc.toString()results in
key: #ccc
valueSo perhaps the problem to fix here is that re-parsing the above result puts the comment as the commentBefore of the value, rather than attaching it as the key comment?
| commentAfterKey?: boolean | ||
| commentOnParentKey?: string | ||
| parentComment?: string | ||
| commentIsAfterKey?: boolean | ||
| context?: any |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| import { prettyToken, tokenType } from './cst.ts' | ||
| import { Lexer } from './lexer.ts' | ||
|
|
||
| const DEBUG = false |
There was a problem hiding this comment.
This is meant to be removed before merging, right?
There was a problem hiding this comment.
Yeah artifact of trying to debug the lib to implement this feature
| const src = `a: b #c\n#d\n` | ||
| const src = source` | ||
| a: b #c | ||
|
|
||
| #d | ||
| ` |
There was a problem hiding this comment.
The previous test had a single newline before #d, why are there now two?
There was a problem hiding this comment.
I can't remember 😅
Something about the trailing comment after all key value pairs is adding 2 newlines
|
@DavidWells Are you still interested in working on this? |
2 participants
Fix for inline comments getting removed #602
It's possible there is an easier way to do this, but this appears to work!
There are a couple of skipped cases that do not pass in tests/trailing-key-comments.test.ts