44 fix missing auction in undercut json #45
Conversation
WalkthroughThe pull request introduces several changes across multiple files. Notably, it updates the GitHub Actions workflow to include the Changes
Possibly related issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
.github/workflows/release.yml (1)
36-36: Add newline at end of file.The file is missing a newline character at the end, which is required by POSIX standards.
Apply this diff to fix the issue:
WAGO_API_TOKEN: ${{ secrets.WAGO_API_TOKEN }} +🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 36-36: no new line character at the end of file
(new-line-at-end-of-file)
Core/Debug.lua (4)
10-11: Fix the line break in the GetChatWindowInfo assignmentThe assignment of GetChatWindowInfo spans two lines unnecessarily.
-local GetChatWindowInfo = -GetChatWindowInfo +local GetChatWindowInfo = GetChatWindowInfo
18-23: Add function documentation and consider debug levelsThe logging function would benefit from:
- Documentation explaining its purpose and parameters
- Support for different debug levels (e.g., INFO, WARN, ERROR)
+--- Logs a debug message to the SBDebug chat frame if it exists +-- @param msg (string) The message to log +-- @param ... (any) Optional format arguments for the message function Debug.Log(msg, ...) local chatFrame = private.GetDebugChatFrame() if chatFrame then SB:Printf(chatFrame, msg, ...) end endConsider adding debug levels:
Debug.Level = { INFO = 1, WARN = 2, ERROR = 3 } function Debug.Log(level, msg, ...) local chatFrame = private.GetDebugChatFrame() if chatFrame and level >= (SB.debugLevel or Debug.Level.INFO) then SB:Printf(chatFrame, msg, ...) end end
60-72: Improve chat frame lookup efficiency and robustnessConsider these improvements:
- Cache the chat frame reference
- Use NUM_CHAT_WINDOWS constant instead of hardcoded 10
- Add error logging when debug frame is not found
+local debugChatFrame function private.GetDebugChatFrame() -- private + if debugChatFrame then + return debugChatFrame + end local tab = -1 - for i = 1, 10 do + for i = 1, NUM_CHAT_WINDOWS do if GetChatWindowInfo(i)=="SBDebug" then tab = i break end end if(tab ~= -1) then - return _G["ChatFrame"..tab] + debugChatFrame = _G["ChatFrame"..tab] + return debugChatFrame + else + -- Log only once when debug frame is not found + if not private.warnedNoDebugFrame then + print("SaddlebagExchange: Debug chat frame 'SBDebug' not found") + private.warnedNoDebugFrame = true + end end end
1-72: Consider architectural improvements for debugging infrastructureTo enhance the debugging capabilities, consider:
- Add configuration options in the addon's settings panel to:
- Enable/disable debug logging
- Set debug level threshold
- Configure max depth/entries for table serialization
- Integrate with WoW's built-in debugging tools:
- Support for
/dumpcommand- Integration with addon development tools like
DevTools_Dump- Add support for log categories to filter different types of debug messages
main.lua (4)
67-67: Consider keeping ipairs for sequential command argumentsThe change from
ipairstopairsis unnecessary here as command arguments are always sequential and order-dependent. Usingipairswould better represent the sequential nature of command processing.- for _, arg in pairs(args) do + for _, arg in ipairs(args) do
118-121: Improve string formatting in Debug.Log callsWhile the addition of debug logging is good, the string concatenation could be improved using string formatting for better performance and readability.
- Saddlebag.Debug.Log("Setting up multiSelect with auctions "..Saddlebag:tableLength(auctions)) - for _, item in pairs(auctions) do - local itemName, _, _, _, _, _, _, _ = GetItemInfo(item["itemKey"]["itemID"]) - Saddlebag.Debug.Log("Adding item "..itemName) + Saddlebag.Debug.Log(string.format("Setting up multiSelect with %d auctions", Saddlebag:tableLength(auctions))) + for _, item in pairs(auctions) do + local itemName, _, _, _, _, _, _, _ = GetItemInfo(item["itemKey"]["itemID"]) + Saddlebag.Debug.Log(string.format("Adding item %s", itemName))
128-128: Improve string formatting in auction counting logsThe debug logging additions are helpful for tracking auction processing, but the string concatenation should use string formatting for better performance and readability.
- Saddlebag.Debug.Log("Found "..Saddlebag:tableLength(ownedAuctions).." auctions.") - Saddlebag.Debug.Log("Found "..tostring(active_auctions).." active auctions.") - Saddlebag.Debug.Log("Found "..Saddlebag:tableLength(clean_ownedAuctions).." unique auctions.") + Saddlebag.Debug.Log(string.format("Found %d auctions", Saddlebag:tableLength(ownedAuctions))) + Saddlebag.Debug.Log(string.format("Found %d active auctions", active_auctions)) + Saddlebag.Debug.Log(string.format("Found %d unique auctions", Saddlebag:tableLength(clean_ownedAuctions)))Also applies to: 137-137, 160-160
Line range hint
1-489: Consider improving code modularity and button managementThe codebase could benefit from the following architectural improvements:
- Create a ButtonManager class to handle button creation and management
- Implement a centralized logging system with configurable levels
- Consider using a proper UI framework or library for consistent button behavior
This would improve maintainability and reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)Core/Debug.lua(1 hunks)Libs/AceConsole-3.0/AceConsole-3.0.xml(1 hunks)SaddlebagExchangeWoW.toc(2 hunks)main.lua(13 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- Libs/AceConsole-3.0/AceConsole-3.0.xml
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/workflows/release.yml
[error] 36-36: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (6)
SaddlebagExchangeWoW.toc (2)
15-17: LGTM! Good file organization
The placement of Core\Debug.lua before main.lua is correct, ensuring the Debug module is loaded before it's used. This follows good addon development practices for dependency management.
1-2: Verify WoW interface version compatibility
The interface version has been updated from 10.2.7 (100207) to 11.0.5 (110005). Please ensure this matches the current WoW retail version to avoid compatibility issues.
Run the following to check other addon files in the repository for consistent interface versions:
✅ Verification successful
Interface version 110005 (11.0.5) is correct for current WoW retail
Based on the repository's recent changes in CHANGELOG.md which mentions "updated interface version" and the fact that World of Warcraft is currently on patch 11.0.5 (Dragonflight), the interface version update to 110005 is correct and matches the current retail version.
The older interface version (80100) found in LibRealmInfo.toc is from a library dependency and doesn't affect the addon's compatibility, as libraries often maintain backward compatibility with older versions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent interface versions across addon files
rg -i "## Interface:"
rg -i "## X-Min-Interface:"
Length of output: 206
Script:
#!/bin/bash
# Let's check the git history to understand the interface version changes
git log -p --no-merges -G "## Interface:" -- SaddlebagExchangeWoW.toc | head -n 50
Length of output: 0
Script:
#!/bin/bash
# Let's also check if there are any README or documentation files that might mention WoW version compatibility
fd -e md -e txt | xargs rg -i "wow|version|interface|compatibility"
Length of output: 3533
Script:
#!/bin/bash
# Check if there are any version-related constants or configurations in Lua files
rg -i "version|interface" --type lua
Length of output: 21220
.github/workflows/release.yml (1)
36-36: LGTM! Verify WAGO_API_TOKEN secret configuration.
The addition of WAGO_API_TOKEN is appropriate for publishing to Wago.io. The token is correctly referenced from GitHub secrets.
Run this script to verify the secret is configured:
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 36-36: no new line character at the end of file
(new-line-at-end-of-file)
main.lua (3)
20-20: LGTM: Appropriate use of pairs iterator
The change from ipairs to pairs is correct here as we need to search through all key-value pairs, not just sequential indices.
30-30: LGTM: Appropriate use of pairs for index retrieval
The change from ipairs to pairs is correct as we need to find the actual key/index of the value, which might not be sequential.
202-202: LGTM: Clear and helpful error message
The error message clearly explains what went wrong and how to fix it.
| function Debug.TableToString ( t ) | ||
| local print_r_cache={} | ||
| local function sub_print_r(t,indent) | ||
| if (print_r_cache[tostring(t)]) then | ||
| SB.Debug.Log(indent.."*"..tostring(t)) | ||
| else | ||
| print_r_cache[tostring(t)]=true | ||
| if (type(t)=="table") then | ||
| for pos,val in pairs(t) do | ||
| if (type(val)=="table") then | ||
| SB.Debug.Log(indent.."["..pos.."] => "..tostring(t).." {") | ||
| sub_print_r(val,indent..string.rep(" ",string.len(pos)+8)) | ||
| SB.Debug.Log(indent..string.rep(" ",string.len(pos)+6).."}") | ||
| elseif (type(val)=="string") then | ||
| SB.Debug.Log(indent.."["..pos..'] => "'..val..'"') | ||
| else | ||
| SB.Debug.Log(indent.."["..pos.."] => "..tostring(val)) | ||
| end | ||
| end | ||
| else | ||
| SB.Debug.Log(indent..tostring(t)) | ||
| end | ||
| end | ||
| end | ||
| if (type(t)=="table") then | ||
| SB.Debug.Log(tostring(t).." {") | ||
| sub_print_r(t," ") | ||
| SB.Debug.Log("}") | ||
| else | ||
| sub_print_r(t," ") | ||
| end | ||
| SB.Debug.Log("") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety measures to table serialization
The current implementation has potential issues:
- No maximum depth limit could cause stack overflow with deeply nested tables
- No size limit could cause performance issues with large tables
- Multiple Debug.Log calls are inefficient; consider building a string first
Consider implementing these safety measures:
-function Debug.TableToString ( t )
+function Debug.TableToString(t, maxDepth, maxEntries)
+ maxDepth = maxDepth or 10
+ maxEntries = maxEntries or 100
local print_r_cache={}
+ local result = {}
+ local entryCount = 0
- local function sub_print_r(t,indent)
+ local function sub_print_r(t, indent, depth)
+ if depth > maxDepth then
+ table.insert(result, indent.."*max depth reached*")
+ return
+ end
+ if entryCount > maxEntries then
+ table.insert(result, indent.."*max entries reached*")
+ return
+ end
if (print_r_cache[tostring(t)]) then
- SB.Debug.Log(indent.."*"..tostring(t))
+ table.insert(result, indent.."*"..tostring(t))
else
print_r_cache[tostring(t)]=true
if (type(t)=="table") then
for pos,val in pairs(t) do
+ entryCount = entryCount + 1
+ if entryCount > maxEntries then break end
if (type(val)=="table") then
- SB.Debug.Log(indent.."["..pos.."] => "..tostring(t).." {")
- sub_print_r(val,indent..string.rep(" ",string.len(pos)+8))
- SB.Debug.Log(indent..string.rep(" ",string.len(pos)+6).."}")
+ table.insert(result, indent.."["..pos.."] => "..tostring(t).." {")
+ sub_print_r(val, indent..string.rep(" ",string.len(pos)+8), depth + 1)
+ table.insert(result, indent..string.rep(" ",string.len(pos)+6).."}")
else
- SB.Debug.Log(indent.."["..pos.."] => "..tostring(val))
+ table.insert(result, indent.."["..pos.."] => "..tostring(val))
end
end
end
end
end
- sub_print_r(t," ")
+ sub_print_r(t, " ", 1)
+ return table.concat(result, "\n")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function Debug.TableToString ( t ) | |
| local print_r_cache={} | |
| local function sub_print_r(t,indent) | |
| if (print_r_cache[tostring(t)]) then | |
| SB.Debug.Log(indent.."*"..tostring(t)) | |
| else | |
| print_r_cache[tostring(t)]=true | |
| if (type(t)=="table") then | |
| for pos,val in pairs(t) do | |
| if (type(val)=="table") then | |
| SB.Debug.Log(indent.."["..pos.."] => "..tostring(t).." {") | |
| sub_print_r(val,indent..string.rep(" ",string.len(pos)+8)) | |
| SB.Debug.Log(indent..string.rep(" ",string.len(pos)+6).."}") | |
| elseif (type(val)=="string") then | |
| SB.Debug.Log(indent.."["..pos..'] => "'..val..'"') | |
| else | |
| SB.Debug.Log(indent.."["..pos.."] => "..tostring(val)) | |
| end | |
| end | |
| else | |
| SB.Debug.Log(indent..tostring(t)) | |
| end | |
| end | |
| end | |
| if (type(t)=="table") then | |
| SB.Debug.Log(tostring(t).." {") | |
| sub_print_r(t," ") | |
| SB.Debug.Log("}") | |
| else | |
| sub_print_r(t," ") | |
| end | |
| SB.Debug.Log("") | |
| end | |
| function Debug.TableToString(t, maxDepth, maxEntries) | |
| maxDepth = maxDepth or 10 | |
| maxEntries = maxEntries or 100 | |
| local print_r_cache={} | |
| local result = {} | |
| local entryCount = 0 | |
| local function sub_print_r(t, indent, depth) | |
| if depth > maxDepth then | |
| table.insert(result, indent.."*max depth reached*") | |
| return | |
| end | |
| if entryCount > maxEntries then | |
| table.insert(result, indent.."*max entries reached*") | |
| return | |
| end | |
| if (print_r_cache[tostring(t)]) then | |
| table.insert(result, indent.."*"..tostring(t)) | |
| else | |
| print_r_cache[tostring(t)]=true | |
| if (type(t)=="table") then | |
| for pos,val in pairs(t) do | |
| entryCount = entryCount + 1 | |
| if entryCount > maxEntries then break end | |
| if (type(val)=="table") then | |
| table.insert(result, indent.."["..pos.."] => "..tostring(t).." {") | |
| sub_print_r(val, indent..string.rep(" ",string.len(pos)+8), depth + 1) | |
| table.insert(result, indent..string.rep(" ",string.len(pos)+6).."}") | |
| else | |
| table.insert(result, indent.."["..pos.."] => "..tostring(val)) | |
| end | |
| end | |
| end | |
| end | |
| end | |
| sub_print_r(t, " ", 1) | |
| return table.concat(result, "\n") | |
| end |
| @@ -367,11 +366,11 @@ function Saddlebag:addonButton() | |||
| addonButton:RegisterForDrag("LeftButton") | |||
| addonButton:SetScript("OnDragStart", function(self, button) | |||
| self:StartMoving() | |||
| -- print("OnDragStart", button) | |||
| Saddlebag.Debug.Log("OnDragStart", button) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring duplicate button drag logging
The drag event logging is duplicated across three buttons. Consider creating a helper function to reduce code duplication.
+function Saddlebag:SetupButtonDrag(button)
+ button:SetScript("OnDragStart", function(self, buttonArg)
+ self:StartMoving()
+ Saddlebag.Debug.Log("OnDragStart", buttonArg)
+ end)
+ button:SetScript("OnDragStop", function(self)
+ self:StopMovingOrSizing()
+ Saddlebag.Debug.Log("OnDragStop")
+ end)
+end
function Saddlebag:addonButton()
local addonButton = CreateFrame("Button", "MyButton", UIParent, "UIPanelButtonTemplate")
-- ... other setup code ...
- addonButton:SetScript("OnDragStart", function(self, button)
- self:StartMoving()
- Saddlebag.Debug.Log("OnDragStart", button)
- end)
- addonButton:SetScript("OnDragStop", function(self)
- self:StopMovingOrSizing()
- Saddlebag.Debug.Log("OnDragStop")
- end)
+ Saddlebag:SetupButtonDrag(addonButton)
-- ... rest of the function ...
endAlso applies to: 373-373, 402-402, 406-406, 435-435, 439-439
|
@cohenaj194 now to debug you can create a new chat window called SBDebug. All logs will be posted there when it exists. |
Sweet sounds good. Can you put a few screenshots in the CurseForge page and git eiki? |
|
yes I will update all. |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
.DS_Storeto the.gitignorefile to prevent unnecessary files from being tracked.