feat(fs): add encoding option to filesystem tools#258
feat(fs): add encoding option to filesystem tools#258Johnsen Young (zhisenyang) wants to merge 7 commits intolangchain-ai:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 79225ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Christian Bromann (christian-bromann)
left a comment
Christian Bromann (christian-bromann)
left a comment
There was a problem hiding this comment.
Changes look good to me. Could we add a unit test that verifies that encoding works properly?
👌 |
Christian Bromann (christian-bromann)
left a comment
Christian Bromann (christian-bromann)
left a comment
There was a problem hiding this comment.
Two more comments
|
|
||
| // Verify raw file content on disk is actually utf-16le encoded | ||
| const diskContentRaw = await fs.readFile(file); | ||
| const decodedDiskContent = Buffer.from(diskContentRaw).toString(encoding); |
There was a problem hiding this comment.
Let's not use Buffer, instead TextDecoder
| }); | ||
|
|
||
| const file = path.join(root, "encoding-test.txt"); | ||
| const text = "Hello encoding with utf-16le"; |
There was a problem hiding this comment.
Looks like we use a utf-16le encoding but a basic utf-8 text - I think we would get more confidence in the test if we also work with text that requires a different encoding
There was a problem hiding this comment.
Thanks for the review! Sure, I’ll add a unit test to verify that the encoding works properly.
|
👌 |
|
Johnsen Young (@zhisenyang) mind addressing above comments? |
2 participants
No description provided.