Fix eight defects found offline, and unbreak the test suite - #263
Open
Pimptasty wants to merge 2 commits into
Open
Fix eight defects found offline, and unbreak the test suite#263Pimptasty wants to merge 2 commits into
Pimptasty wants to merge 2 commits into
Conversation
Each of these fails silently: nothing raises where a user would see it, so the
symptom is a feature that quietly does nothing. Regression tests accompany every
one, in tests/.
Config.lua
reload() called C_GuildInfo.GuildRoster() bare while tests/TestConfig.lua
asserts a fallback to the global, so the suite is currently red on this branch.
Restored the feature-detect, which also protects a client that lacks the
namespace: it raises there on the first configuration load, before the bridge
comes up.
A GW:v: line the guard accepted but semver() rejects aborted the entire parse.
'^%d+%.%d+%.%d+%w*$' allows '1.2.3beta', which is not a semantic version (a
pre-release needs the '-'), so semver() returned nothing and tostring() raised
with no argument -- unwinding out of load() before the channel, peer and
officer directives on the following lines were read. One mistyped character in
the Guild Information panel took the whole confederation's configuration down.
Now validated by parsing; a bad version is logged and skipped.
is_container() opened with self:GetGuildName(), which GwConfig does not define,
so every call raised. The helper is gw.GetGuildName().
Settings.lua
reset() required an svtable, but GreenWallInterfaceFrame_SetDefaults calls it
with none -- the Interface Options "Defaults" button raised on the first key.
Underneath, the body only assigned a default when the stored value was absent
or invalid. That guard belongs to initialize(), which fills gaps; reset() must
overwrite values that are present and valid, so it was a no-op on any real
store even when called correctly.
API.lua
A handler registered with '*' never received anything. The dispatcher tested
whether the SENDING addon was named '*', but AddMessageHandler and SendMessage
both assert the addon against C_AddOns.GetAddOnInfo, so no real sender can be.
The wildcard belongs to the registration, as API.md documents.
Channel.lua
The bridge channel was never hidden. join() scanned GetChatWindowMessages()
for the channel name, but that returns message GROUP names (GUILD, SAY); the
channel list is GetChatWindowChannels(). Blizzard reads the two into
RegisterForMessages and RegisterForChannels respectively. A group name can
never equal a channel name, so the loop matched nothing.
Had it matched it would have raised: ChatFrame_RemoveChannel is a deprecation
alias for ChatFrameMixin.RemoveChannel, which takes the frame as self, and the
call passed the frame's NAME. Also iterates NUM_CHAT_WINDOWS rather than a
hard-coded 10.
HoldDown.lua
start() created its OnUpdate frame into a bare local. Nothing else referenced
it -- the handler takes frame as a parameter rather than capturing it -- so
the timer's survival depended on the client never reclaiming the frame, and a
timer that stops firing produces no error at all. The flip side is a leak:
refresh_channels restarts these for the whole session and each start created a
new frame. Created once, kept on the instance, reused.
Lib/SemanticVersion.lua
Three defects in pre-release comparison, none of which had ever executed:
- A version with build metadata but no pre-release was rejected outright. The
suffix pattern required a leading '-', so '1.0.0+20130313144700', valid per
semver 2.0.0 section 10, parsed to nothing.
- The comparison was not antisymmetric: alphanumeric-versus-numeric returned
"less than" in both directions, so 1.0.0-alpha < 1.0.0-1 and 1.0.0-1 <
1.0.0-alpha were both true and any ordering depended on argument order.
- A shorter pre-release outranked a longer one, making 1.0.0-alpha rank above
1.0.0-alpha.1 -- the reverse of the specification's own worked example, which
tests/TestSemanticVersion.lua now walks end to end.
VERSION_MINOR goes 1 -> 2, and that is load-bearing rather than bookkeeping:
LibStub:NewLibrary returns nil when oldminor >= minor, so at an unchanged minor
this corrected copy would lose to any other add-on carrying an older copy that
happened to load first, and the fixes would ship without ever running.
gw.usage did not list mode, joindelay or refresh, so three working commands were undiscoverable in game -- /gw help is the only place they are advertised. README.md documented a "stats" command. There has never been one; the slash handler has no such branch, so anyone following the documentation got "Unknown command: stats".
Pimptasty
requested review from
StupidGenius,
andyvanhout and
mrogaski
as code owners
August 6, 2026 04:47
mrogaski
requested changes
Aug 25, 2026
mrogaski
left a comment
Member
There was a problem hiding this comment.
Please submit each of the fixes as a separate pull request. With all eight fixes in one commit, it's difficult to isolate any regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eight defects, each found by running the existing
tests/suite offline and writing a test forwhat the code should do. Every fix has a regression test in this PR.
The suite on this branch is currently red — two errors in
TestConfig.GwConfig:reload()calls
C_GuildInfo.GuildRoster()bare, whiletests/TestConfig.luaasserts a fallback to theglobal. The first commit restores the feature-detect so the two agree; it also protects a client
without the namespace, which raises there on the very first configuration load, before the bridge
comes up.
The rest
GW:v:could abort the entire configuration parse. The guard'^%d+%.%d+%.%d+%w*$'accepts1.2.3beta, which is not a semantic version — a pre-release needsthe
-.semver()returns nothing,tostring()then raises with no argument, andload()unwinds before the channel, peer and officer directives on the following lines are read. One
mistyped character in the Guild Information panel takes a whole confederation's configuration
down. Now validated by parsing rather than by regex.
GwConfig:is_container()raised on every call —self:GetGuildName(), whichGwConfigdoesnot define. The helper is
gw.GetGuildName().The Interface Options "Defaults" button raised.
GwSettings:reset()requires ansvtable,but
GreenWallInterfaceFrame_SetDefaultscalls it with none. Underneath sat a second defect:the body only assigned a default when the stored value was absent or invalid — a guard that
belongs to
initialize(), makingreset()a no-op on any real store even when called correctly.API handlers registered with
'*'never received anything. The dispatcher tested whether thesending addon was named
'*', butAddMessageHandlerandSendMessageboth assert the addonagainst
C_AddOns.GetAddOnInfo, so no real sender can be. The wildcard belongs to theregistration, as
API.mddocuments.The bridge channel was never hidden.
GwChannel:join()scansGetChatWindowMessages()forthe channel name, but that returns message group names (
GUILD,SAY) — the channel list isGetChatWindowChannels(). Blizzard reads the two intoRegisterForMessagesandRegisterForChannelsrespectively, so a group name can never equal a channel name and the loopmatches nothing. Had it matched it would have raised:
ChatFrame_RemoveChannelis a deprecationalias for
ChatFrameMixin.RemoveChannel, which takes the frame asself, and the call passed theframe's name.
Hold-down timers could stop firing silently, and leaked a frame per start.
GwHoldDown:start()created its
OnUpdateframe into a bare local; nothing else referenced it, because the handlertakes
frameas a parameter rather than capturing it. A timer that stops produces no error atall. Created once, kept on the instance, reused.
Three pre-release precedence defects in
Lib/SemanticVersion.lua, in code that had neverexecuted: a version with build metadata but no pre-release (
1.0.0+20130313144700, valid persemver 2.0.0 §10) was rejected outright; the comparison was not antisymmetric, so
1.0.0-alpha < 1.0.0-1and1.0.0-1 < 1.0.0-alphawere both true; and a shorter pre-releaseoutranked a longer one, making
1.0.0-alpharank above1.0.0-alpha.1.TestSemanticVersion.luawalks the specification's own worked example.
VERSION_MINORgoes 1 → 2, which is load-bearing rather than bookkeeping:LibStub:NewLibraryreturns nil whenoldminor >= minor, so at an unchanged minor this correctedcopy would lose to any other add-on carrying an older copy that loaded first, and the fixes would
ship without ever running.
Docs
gw.usagedid not listmode,joindelayorrefresh, so three working commands wereundiscoverable in game.
README.mddocumented astatscommand that has never existed — the slashhandler has no such branch.
Testing
All seven test files pass on this branch. luaunit is not vendored here, so:
LUA_PATH="./?.lua;./tests/?.lua;/path/to/luaunit/?.lua;;" lua tests/TestSemanticVersion.luaThe
SemanticVersiontests fail 5/17 against the unfixed library and pass 17/17 with the fix.Same change is offered against
main,release/classicandrelease/classic-era.