I have a fairly large piece of work sitting on top of master and I would like to contribute it,
but the shape of it needs a decision from you before I open anything.
Short version: it is 55 commits across seven branches, it changes public API, and I do not think
it belongs in master one piece at a time. I would rather build it with you on a branch than
hand it over as a finished thing.
What it is
Seven branches, each based on the previous, all rebased onto master at d0289e6:
|
Branch |
What it does |
Diff |
| 0 |
pr/0-poll-deadline |
Polling helpers give up one attempt too early |
3 files, +35 −13 |
| 1 |
pr/1-mode-architecture |
Environment modes, matrix runs, plugin host, gradle module split |
70 files, +4812 −1322 |
| 2 |
pr/2-plugin-packages |
@plugwright/auth-authme, @plugwright/console-rcon, and the runner API they needed |
49 files, +2475 −57 |
| 3 |
pr/3-workspace-layout |
Fixed tests/ plugins/ dist/ generated/ layout, with migration |
54 files, +659 −169 |
| 4 |
pr/4-npm-registries |
Declare npm registries in the build script, generate .npmrc |
13 files, +580 −9 |
| 5 |
pr/5-player-reuse |
Reuse a connected player across tests, reuseTest |
32 files, +1102 −112 |
| 6 |
pr/6-publishing |
Publish all three npm packages and the plugin, publicly or to your own registry |
10 files, +541 −33 |
Every link above is the diff of that branch against the one before it, so each row is the actual
change rather than a running total. Every branch builds clean and the example plugin's suite
passes on the tip: 47 passed, 0 failed.
What I am asking for
A long-lived branch in this repo — next, 3.0, whatever you want to call it. Two reasons.
The practical one: GitHub requires a PR's base branch to live in the base repository. With only
master to point at, branches 1 through 6 would each have to target it, and every PR would show
its own change plus everything under it. The sixth would be a 55-commit diff. The split that makes
this reviewable only works if the branches exist on your side.
The real one: I do not think this is finished, and I would rather not pretend it is. The mode API,
the plugin host contract and the reuse system all have rough edges I already know about —
reuseTest only supports one setup step per pool, which is the wrong shape and wants
requires/abilities conditions on several. Working through those will change public API more than
once. Merged into master piecemeal, that means either breaking 2.x users repeatedly or freezing
decisions early to avoid it. On a branch, master keeps shipping 2.x while this settles.
Whatever comes out the far end can be 3.0.0 or any number you prefer. Nothing in the series assumes
one — there is no version bump anywhere in it, version.txt stays at 2.0.4-dev.0, and the README
and quickstart keep the 2.0.3 they already show.
Say the word and I will re-point all six and open them in order, each with its own description.
One thing that is ready now
#45 is separate and does not need any of this. It fixes a bug already on master: every polling
helper checks the deadline instead of making a final attempt, so anything that arrives during
the last sleep is never looked at. It is opened against master and can be merged whenever.
About your last two commits
The series was rebased onto them rather than around them, and one of them changed my mind.
c4a89a4 and my branch 1 solved the same problem differently — you moved the message buffer onto
PlayerWrapper, I had moved it onto a session object. Yours is right and mine was not: per-session
is still one buffer shared by every bot, so an assertion on player A could be satisfied by
something player B heard. Renaming the global is not fixing it. The series now uses your per-player
buffers, and three things fell out of that: AdminBotConsole stopped hand-rolling a private buffer
plus a duplicate listener, the AuthMe plugin matches prompts against its own player, and
disconnectAllBots delegates to disconnectBot so your removeAllListeners runs on every path.
There is one case your patch does not reach, because the feature that creates it is in branch 5: a
player checked out under stay never leaves, so it never rejoins and never gets the clear. Fixed
there.
From d0289e6, the shared task setup, typed TaskProviders, the verification group and the
banner all already existed in the series independently. The IDEA sync trigger would have been
dropped silently by the module split, so it is carried over in its own commit and now triggers the
compile task, which installs and compiles. Your up-to-date checks on plugwrightNpmInstall are
not carried over, and I would rather say so than let you find it: that task merged into
PlugwrightCompileTestsTask, which declares outputs.upToDateWhen { false } on purpose, because
its output depends on node_modules and on the installed runner package and neither is a declared
input. Making it properly incremental is worth doing and was not something I wanted to rush inside
a rebase.
Happy to split, reorder or drop any of it. Tell me what you actually want and I will reshape it.
I have a fairly large piece of work sitting on top of
masterand I would like to contribute it,but the shape of it needs a decision from you before I open anything.
Short version: it is 55 commits across seven branches, it changes public API, and I do not think
it belongs in
masterone piece at a time. I would rather build it with you on a branch thanhand it over as a finished thing.
What it is
Seven branches, each based on the previous, all rebased onto
masteratd0289e6:pr/0-poll-deadlinepr/1-mode-architecturepr/2-plugin-packages@plugwright/auth-authme,@plugwright/console-rcon, and the runner API they neededpr/3-workspace-layouttests/plugins/dist/generated/layout, with migrationpr/4-npm-registries.npmrcpr/5-player-reusereuseTestpr/6-publishingEvery link above is the diff of that branch against the one before it, so each row is the actual
change rather than a running total. Every branch builds clean and the example plugin's suite
passes on the tip:
47 passed, 0 failed.What I am asking for
A long-lived branch in this repo —
next,3.0, whatever you want to call it. Two reasons.The practical one: GitHub requires a PR's base branch to live in the base repository. With only
masterto point at, branches 1 through 6 would each have to target it, and every PR would showits own change plus everything under it. The sixth would be a 55-commit diff. The split that makes
this reviewable only works if the branches exist on your side.
The real one: I do not think this is finished, and I would rather not pretend it is. The mode API,
the plugin host contract and the reuse system all have rough edges I already know about —
reuseTestonly supports one setup step per pool, which is the wrong shape and wantsrequires/abilities conditions on several. Working through those will change public API more thanonce. Merged into
masterpiecemeal, that means either breaking 2.x users repeatedly or freezingdecisions early to avoid it. On a branch,
masterkeeps shipping 2.x while this settles.Whatever comes out the far end can be 3.0.0 or any number you prefer. Nothing in the series assumes
one — there is no version bump anywhere in it,
version.txtstays at2.0.4-dev.0, and the READMEand quickstart keep the
2.0.3they already show.Say the word and I will re-point all six and open them in order, each with its own description.
One thing that is ready now
#45 is separate and does not need any of this. It fixes a bug already on
master: every pollinghelper checks the deadline instead of making a final attempt, so anything that arrives during
the last sleep is never looked at. It is opened against
masterand can be merged whenever.About your last two commits
The series was rebased onto them rather than around them, and one of them changed my mind.
c4a89a4and my branch 1 solved the same problem differently — you moved the message buffer ontoPlayerWrapper, I had moved it onto a session object. Yours is right and mine was not: per-sessionis still one buffer shared by every bot, so an assertion on player A could be satisfied by
something player B heard. Renaming the global is not fixing it. The series now uses your per-player
buffers, and three things fell out of that:
AdminBotConsolestopped hand-rolling a private bufferplus a duplicate listener, the AuthMe plugin matches prompts against its own player, and
disconnectAllBotsdelegates todisconnectBotso yourremoveAllListenersruns on every path.There is one case your patch does not reach, because the feature that creates it is in branch 5: a
player checked out under
staynever leaves, so it never rejoins and never gets the clear. Fixedthere.
From
d0289e6, the shared task setup, typedTaskProviders, theverificationgroup and thebanner all already existed in the series independently. The IDEA sync trigger would have been
dropped silently by the module split, so it is carried over in its own commit and now triggers the
compile task, which installs and compiles. Your up-to-date checks on
plugwrightNpmInstallarenot carried over, and I would rather say so than let you find it: that task merged into
PlugwrightCompileTestsTask, which declaresoutputs.upToDateWhen { false }on purpose, becauseits output depends on
node_modulesand on the installed runner package and neither is a declaredinput. Making it properly incremental is worth doing and was not something I wanted to rush inside
a rebase.
Happy to split, reorder or drop any of it. Tell me what you actually want and I will reshape it.