Update dependency yaml to v2 - #301
Conversation
| 6 | ¶ | ||
| | ^ | ||
| <blockFolded chomping="keep" indent=1 value=" 123\\n 456\\n\\n\\n" /> | ||
| <blockFolded chomping="keep" indent=1 value=" 123\\n 456\\n\\n\\n" /> |
There was a problem hiding this comment.
The new value looks correct to me. What was the original value intended?
There was a problem hiding this comment.
I think you are right, should we use blockValue.value here? https://github.com/ota-meshi/yaml-unist-parser/blob/c2eef11612275700dbfc796c5c910e8ee09c1f3f/src/transforms/block-value.ts#L46
There was a problem hiding this comment.
I tried changing it to String(blockValue.value), but it doesn't seem to make any difference to blockValue.source 🤔
There was a problem hiding this comment.
Also, String(blockValue.value) seems to break if there is a rather !!binary tag.
There was a problem hiding this comment.
I think we can trust .value in this case.
There was a problem hiding this comment.
Some expected values in the yaml-test-suite can be found in the test,
eg: JEF9-2.yaml
https://github.com/yaml/yaml-test-suite/blob/ccfa74e56afb53da960847ff6e6976c0a0825709/src/JEF9.yaml#L40
There was a problem hiding this comment.
As far as I can see, the new values are the same as the yaml tests.
|
Can you try to fix the build script first? So we can have a test in Prettier, see how many tests would fail. |
|
I've reviewed the last few remaining diffs, and the new values seem reasonable to me. Could you please check these diffs? Please let me know if there are any issues. |
What kind of script is that? Can you tell me more about it? |
|
Oh, I hadn't checked the coverage yet. |
|
All diff looks good to me! Thank you very much! |
commit: |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| getOrderedComments(): Comment[] { | ||
| return this.#comments.sort( | ||
| (a, b) => a.position.start.offset - b.position.start.offset, | ||
| ); |
There was a problem hiding this comment.
Tests still passing without .sort(), can you confirm if it is still needed, or find a way to test if it does.
There was a problem hiding this comment.
Ah, there is another sort in comment attach.
There was a problem hiding this comment.
That's interesting.
Maybe we already sorted it somewhere else. I'll look into it.
There was a problem hiding this comment.
I removed the unnecessary sorting.
We were already sorting somewhere else.
yaml-unist-parser/src/attach.ts
Line 30 in fbcd4e4
| } | ||
| } | ||
| // istanbul ignore next | ||
| // istanbul ignore next -- @preserve |
There was a problem hiding this comment.
It seems that @preserve is needed to make this comment visible in the transpiled code by vitest.
There was a problem hiding this comment.
Don't worry about the coverage. You can ignore.
|
Thanks again! ❤️ |
|
@ota-meshi Do you think we can build something that can be used in both this package and |
|
Thank you for the review and the refactoring after the merge!
I'm not sure yet 😅 If we need it, I'd love to share it with you. |
2 participants
This PR upgrades the
yamlpackage to v2.I think most of the issues are with node locations, but there seem to be some differences in scalar values such as block literals. Also, I'm not sure how to handle the new options yet.close #279