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 enhances the 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 JSON output for xmake show commands, particularly for target information. The changes are well-structured, separating data collection from presentation, which is a good design choice. A new string.serializable helper has been added to prepare Lua objects for JSON encoding. My review includes suggestions to remove some minor code redundancies, fix a small regression in the human-readable output, and a refactoring suggestion to improve maintainability.
xmake/plugins/show/info/target.lua
Outdated
| local sourceinfo = _get_sourceinfo(opt_, name, value, {tips = tips}) | ||
| values[value] = sourceinfo or {tips = tips} |
There was a problem hiding this comment.
The or {tips = tips} part of this assignment is redundant. The _get_sourceinfo function will always return a table when tips is provided, as it is in this context. It will never return nil, so the fallback is unnecessary.
values[value] = _get_sourceinfo(opt_, name, value, {tips = tips})
| local deps = target:get("deps") | ||
| if deps then | ||
| cprint(" ${color.dump.string}deps${clear}:") | ||
| local entries = {} | ||
| for _, dep in ipairs(deps) do | ||
| cprint(" ${color.dump.reference}->${clear} %s%s", dep, _get_sourceinfo_str(target, "deps", dep)) | ||
| local entry = {name = dep, source = _get_sourceinfo(target, "deps", dep)} | ||
| table.insert(entries, entry) | ||
| end | ||
| if #entries > 0 then | ||
| info.deps = entries | ||
| end | ||
| end |
There was a problem hiding this comment.
There is some code duplication in how deps, rules, and packages are collected. The logic for iterating over target:get(key), creating entries, and adding them to the info table is very similar for these three.
Consider creating a helper function to reduce this repetition, for example:
local function _collect_simple_entries(target, info, key)
local values = target:get(key)
if not values then return end
local entries = {}
for _, value in ipairs(values) do
table.insert(entries, {name = value, source = _get_sourceinfo(target, key, value)})
end
if #entries > 0 then
info[key] = entries
end
endYou could then call this for deps, rules, and packages, which would make the _collect_target_info function shorter and more maintainable.
| end | ||
| if info.compilers then | ||
| for _, compiler in ipairs(info.compilers) do | ||
| cprint(" ${color.dump.string}compiler (%s)${clear}: %s", compiler.sourcekind, compiler.program) |
There was a problem hiding this comment.
The human-readable output for compilers is missing the general compiler flags (compiler.flags). The previous implementation displayed them. To restore this information and for consistency with how linker flags are displayed, you should also print compiler.flags here.
cprint(" ${color.dump.string}compiler (%s)${clear}: %s", compiler.sourcekind, compiler.program)
cprint(" ${color.dump.reference}->${clear} %s", compiler.flags)
xmake/plugins/show/info/target.lua
Outdated
| local info = _collect_target_info(target) | ||
| if opt.json then | ||
| info.api_entries = nil | ||
| print(json.encode(string.serializable(info) or {})) |
There was a problem hiding this comment.
The _collect_target_info function is designed to produce a table that is already serializable by calling string.serializable on potentially problematic values. Therefore, the outer string.serializable(info) call is likely redundant and adds a performance overhead by traversing the entire info table again. Additionally, since info is always a table, or {} is also unnecessary. You can simplify this line.
print(json.encode(info))
Uh oh!
There was an error while loading. Please reload this page.