Conversation
Summary of ChangesHello @waruqi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new XML module, providing functionality for decoding and encoding XML strings and files. The module defines a clear node structure and includes helper functions for common XML operations like finding nodes and extracting text. Unit tests have been added to cover basic decoding, encoding, and special node types. The sandbox wrapper correctly integrates the new module and handles errors by raising exceptions. Overall, the implementation is a good foundation for XML processing.
xmake/core/base/xml.lua
Outdated
| table.insert(children, xml.cdata(value)) | ||
| i = close + 3 | ||
| elseif data:sub(lt + 1, lt + 8):upper() == "!DOCTYPE" then | ||
| local close = data:find(">", lt + 8) |
There was a problem hiding this comment.
The string.find function is used here without the plain argument (true). This means the > character in the pattern is treated as a magic character for Lua patterns, which is not the intended behavior. It should be data:find(">", lt + 8, true) to ensure > is matched literally. This issue appears in multiple places within the decode function.
local close = data:find(">", lt + 8, true)
xmake/core/base/xml.lua
Outdated
| end | ||
| i = close + 2 | ||
| elseif data:sub(lt + 1, lt + 1) == "!" then | ||
| local close = data:find(">", lt + 2) |
There was a problem hiding this comment.
Similar to the previous comment, the string.find function is used here without the plain argument (true). This could lead to incorrect parsing if the > character is interpreted as a pattern magic character. It should be data:find(">", lt + 2, true) to match > literally.
local close = data:find(">", lt + 2, true)
xmake/core/base/xml.lua
Outdated
| end | ||
| i = close + 1 | ||
| elseif data:sub(lt + 1, lt + 1) == "/" then | ||
| local close = data:find(">", lt + 1) |
There was a problem hiding this comment.
The string.find function is used here without the plain argument (true). This means the > character in the pattern is treated as a magic character for Lua patterns, which is not the intended behavior. It should be data:find(">", lt + 1, true) to ensure > is matched literally.
local close = data:find(">", lt + 1, true)
xmake/core/base/xml.lua
Outdated
| end | ||
| i = close + 1 | ||
| else | ||
| local close = data:find(">", lt + 1) |
There was a problem hiding this comment.
The string.find function is used here without the plain argument (true). This means the > character in the pattern is treated as a magic character for Lua patterns, which is not the intended behavior. It should be data:find(">", lt + 1, true) to ensure > is matched literally.
local close = data:find(">", lt + 1, true)
xmake/core/base/xml.lua
Outdated
| function xml._parse_attrs(attrstr) | ||
| local attrs | ||
| attrstr:gsub("([%w_:%-%.]+)%s*=%s*([\"'])(.-)%2", function(key, quote, value) | ||
| attrs = attrs or {} | ||
| attrs[key] = xml._decode_entities(value) | ||
| end) |
There was a problem hiding this comment.
The regular expression for parsing attributes expects values to be enclosed in either double or single quotes. However, XML attribute values can also be unquoted if they do not contain spaces or special characters. This current implementation might fail to parse valid XML where attributes are unquoted, for example, <element attr=value>. Consider expanding the regex to support unquoted attribute values for broader XML compatibility.
xmake/core/base/xml.lua
Outdated
| if opt.trim_text ~= false then | ||
| text = text:gsub("^%s+", ""):gsub("%s+$", "") |
There was a problem hiding this comment.
The trim_text option defaults to true (because opt.trim_text ~= false evaluates to true if opt.trim_text is nil). While trimming whitespace is often desired, it can lead to data loss if significant whitespace needs to be preserved, such as in xml:space="preserve" contexts. It would be more explicit and safer to make trim_text false by default and require users to opt-in for trimming, or provide a clear option to disable it when necessary.
core.base.xml
The
core.base.xmlmodule provides a tiny DOM-style XML toolkit that works inside Xmake’s sandbox. It focuses on predictable data structures, JSON-like usability, and optional streaming so you can parse large XML documents without building the entire tree.Node Structure
XML nodes are plain Lua tables. All constructors (
xml.new,xml.text,xml.comment, etc.) return values shaped like:{ name = "element-name" | nil, -- only for element nodes kind = "element" | "text" | "comment" | "cdata" | "doctype" | "document", attrs = { key = value, ... } or nil, text = string or nil, children = { child1, child2, ... } or nil, prolog = { comment/doctype nodes before root } or nil }Because these are regular tables, mutating them updates the DOM in place and the changes show up automatically when you call
xml.encodeorxml.savefile.Quick Start
Streaming Example
xml.scanwalks nodes as they are completed; returningfalsestops the scan immediately. This is ideal for large files (e.g. Info.plist) when you only need a few entries.Options Summary
trim_text = truexml.decode,xml.scankeep_whitespace_nodes = truexml.decode,xml.scantrim_textproduced non-empty content).pretty = true/indent/indentcharxml.encode,xml.savefileAPI Reference
xml.new(opt)Create a custom node.
optmay containname,kind,attrs,children, andtext. Usually you call the dedicated helpers below instead ofxml.newdirectly.Element/Text Helpers
All helpers return node tables that you can insert into
children.xml.decode(data, opt)Parse an XML string into a node tree. Returns the single root element when there is exactly one element, or all top-level nodes when multiple elements exist. On failure returns
nil, err.Supports:
root.prologwhen present).<item flag=true path=/tmp/file>.name,attrs,children).trim_textandkeep_whitespace_nodesoptions described above.xml.encode(node, opt)Serialize a node tree back into XML. Set
{pretty = true, indent = 2}for multi-line output or pass a customindentchar.xml.loadfile(path, opt)/xml.savefile(path, node, opt)Convenience wrappers that call
io.readfile/io.writefileand reuse the decode/encode options.xml.text_of(node)Concatenate all direct text children and return the combined string. Useful for quickly reading
<string>...</string>values.xml.find(node, path)XPath-like lookup supporting:
/child axis,//descendant axis.*) and node tests (text(),comment(),cdata(),doctype()).[@id='foo'],[@enabled]), text predicates ([text()='value']), positional indexes ([2]).Returns the first node that matches or
nilif nothing is found.xml.scan(data, callback, opt)Streaming parser. Calls
callback(node)for each completed node; returningfalsestops the scan early. Accepts the same options asxml.decode(trim_text,keep_whitespace_nodes). Nodes produced byxml.scanshare the same structure asxml.decode.Attribute Parsing Notes
a="1 2",b='foo',c=bare).&→&).Example: Parsing and Updating an Info.plist
This example demonstrates decoding, querying via DOM traversal, mutating nodes, and writing the file back with pretty formatting.