Skip to content

Conversation

@samhvw8
Copy link
Contributor

@samhvw8 samhvw8 commented Feb 3, 2025

  • Add fast-xml-parser dependency for XML parsing
  • Create xml utility module with parseXml function
  • Update insert-content and search-and-replace tools to use XML format
  • Modify Cline.ts to parse operations as XML instead of JSON

Description

Motivation: LLM is bad at generate JSON https://aider.chat/2024/08/14/code-in-json.html

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist:

  • My code follows the patterns of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Additional context

Related Issues

Reviewers


Important

Refactor code to switch from JSON to XML format for tool operations, updating parsing and tool descriptions accordingly.

  • Behavior:
    • Switch from JSON to XML for tool operations in Cline.ts, affecting insert-content and search-and-replace tools.
    • Modify Cline.ts to parse operations as XML using parseXml().
  • Utilities:
    • Add parseXml() function in xml.ts for XML parsing.
    • Add fast-xml-parser dependency in package.json.
  • Prompts:
    • Update insert-content.ts and search-and-replace.ts to reflect XML format in tool descriptions.

This description was created by Ellipsis for 177b5eb6d962aa40bdb6f5f4becc463e85f97728. It will automatically update as commits are pushed.

@changeset-bot
Copy link

changeset-bot bot commented Feb 3, 2025

⚠️ No Changeset found

Latest commit: 3760369

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@wwicak
Copy link

wwicak commented Feb 3, 2025

Instead of full XML conversion, consider:

  1. Optimizing JSON operations with streaming parsers
  2. Enhancing the current XML-like tag system with validation
  3. Implementing structural hashing for diff operations
  4. Moving to Protocol Buffers if binary efficiency is needed

@samhvw8
Copy link
Contributor Author

samhvw8 commented Feb 3, 2025

Instead of full XML conversion, consider:

  1. Optimizing JSON operations with streaming parsers
  2. Enhancing the current XML-like tag system with validation
  3. Implementing structural hashing for diff operations
  4. Moving to Protocol Buffers if binary efficiency is needed

@wwicak thanks for you comment, but

  1. streaming parsers is not work because another tool line read_file and write_to_file not use json, only this 2 tool (search_and_replace and insert_content)
  2. already has validation when streaming parse
  3. not related to this PR, you can contribute this PR if you have time
  4. LLM can not return to binary to use binary buffer

Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we do any escaping of the XML content? What will happen if we try to replace the term </search>?

src/utils/xml.ts Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrubens this config to escape xml content

Comment on lines 1742 to 1869
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrubens i will pass to it like this
[
"operation.search",
"operation.replace",
"operation.regex_flags",
]

@samhvw8
Copy link
Contributor Author

samhvw8 commented Feb 10, 2025

@mrubens anything i should do for this o.o

@bramburn
Copy link
Contributor

the problem with using XML is that some smaller language model are not good with XML, but good with JSON, would it be worth allow for different options?

@samhvw8
Copy link
Contributor Author

samhvw8 commented Feb 10, 2025

the problem with using XML is that some smaller language model are not good with XML, but good with JSON, would it be worth allow for different options?

@bramburn i was thinking that an option we can change how Roo use tool ( via tool use, via xml, via json ... )
what is your thoughts about that ?

@bramburn
Copy link
Contributor

That's another pr. Probably will take weeks

@bramburn
Copy link
Contributor

bramburn commented Feb 11, 2025 via email

@samhvw8 samhvw8 force-pushed the feat/convert-2-tool-to-xml branch from c2dc9ad to f387832 Compare March 3, 2025 05:08
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Mar 3, 2025
@hannesrudolph hannesrudolph moved this to To triage in Roo Code Roadmap Mar 5, 2025
@hannesrudolph hannesrudolph moved this from To triage to PR - Needs Approval in Roo Code Roadmap Mar 6, 2025
@mrubens mrubens moved this from PR [Unverified] to PR [Deferred] in Roo Code Roadmap Mar 10, 2025
@samhvw8 samhvw8 force-pushed the feat/convert-2-tool-to-xml branch from f387832 to 3760369 Compare April 11, 2025 10:19
@samhvw8 samhvw8 closed this Apr 12, 2025
@github-project-automation github-project-automation bot moved this from PR [Deferred] to Done in Roo Code Roadmap Apr 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants