chore: drop the compatibility shims nobody is using - #73
Merged
Conversation
Two shims kept for users who do not exist yet, plus a wording fix. The tool has no installed base, which is the window in which removing a compatibility path costs nothing. It does not stay open. The per-tool cookie variable. When the suite was a set of standalone binaries each read its own RBX<TOOL>_COOKIE; the merge into `rbx <subcommand>` unified those, and one survivor was kept on the argument written into its own doc comment: removing a variable that works would break whoever set it. Nobody set it. It cost a branch in the chain that resolves a full-account credential, and the one entry in STILL_READ, so that check now has no exceptions at all. Two allow(unsafe_code) go with it: they were there because the tests called set_var, and both modules now work off the thread-local Studio seam instead. That variable was also the only cookie source reachable from outside rbx-core, which made it the only way to test that a caller resolves the cookie rather than reading the raw flag. rbx-import needed that: keying the meta-gap report off `--cookie` alone told everyone with Studio signed in to re-run for fields meta had just read. So `meta_gaps` now takes the resolved cookie instead of the flags, moving the distinction to its single call site where it reads as `resolve_cookie()` and the bug would have to be written in plain sight. `rbx data delete` and `restore`. 0.7.0 renamed these `delete-key` and `restore-key` so the level would be in the name beside `delete-store`, then kept the old spellings as aliases, holding open the exact ambiguity the rename closed, on the one pair of subcommands where reading the wrong level destroys the wrong thing. The test that pinned them is inverted rather than deleted: an alias is one attribute and comes back by reflex. `download` no longer calls its default backend legacy. The word was about Roblox's older assetdelivery host, but on the default path it reads as a deprecation notice, and that backend is the default precisely because it reaches more assets than Open Cloud does.
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.
chore: drop the compatibility shims nobody is using
Two shims kept for users who do not exist yet, plus a wording fix.
The tool has no installed base. That is the window in which removing a
compatibility path costs nothing, and it does not stay open: every one of these
gets more expensive to remove the day somebody depends on it, and none of them
gets cheaper to keep.
The per-tool cookie variable
When the suite was a set of standalone binaries, each read its own
RBX<TOOL>_COOKIE. Folding them intorbx <subcommand>unified those toRBX_COOKIE. One survivor was kept, on the argument written into its own doccomment: removing a variable that works today would break whoever set it.
Nobody set it. What it cost:
GlobalFlags::resolve_cookie, the chain that resolves afull-account credential, where the ordering between explicit sources,
--no-auto-cookieand Studio auto-detection is already subtle enough to haveproduced fix(ban):
listhonours --limit, and the walk cannot spin #20;STILL_READ, the exception list intests/env_var_names.rs. That check now has no exceptions, which is thestate an exception list should be in: one entry invites a second.
Two
#[allow(unsafe_code)]go with it. They existed because the tests set thevariable, and
set_varisunsafeunder the 2024 semantics. Both test modulesnow work off the thread-local Studio seam instead, so the allow would only be
sitting there permitting the next
set_varto come back.The one thing this cost
The variable was the only cookie source reachable from outside
rbx-core,which made it the only way to write a test distinguishing
resolve_cookie()from the raw
--cookieflag.rbx-importused it for exactly that:meta_gapsdecides whether to tell the user "re-run with
--cookie" for fields the metastep may have just read correctly, and keying that off the flag alone told
everyone with Studio signed in to redo finished work.
So
meta_gapsnow takes the resolved cookie rather than the flags to resolve itfrom:
The distinction it got wrong once moved to its single call site, where it reads
env_global.resolve_cookie().as_deref()and re-introducing the bug meanswriting
env_global.cookieon a line that plainly looks wrong. The test istotal instead of illustrative: both inputs, no privileged source.
rbx data deleteandrestore0.7.0 renamed these to
delete-keyandrestore-key, so the level would be inthe name beside
delete-storeandrestore-store. Its own doc comment sayswhy:
delete Player_156anddelete-store PlayerDataare one glance apart in ashell history, and only one of them is recoverable by re-running the game.
Then it kept the old spellings as aliases, which held open the exact ambiguity
the rename existed to close, on the one pair of subcommands here where reading
the wrong level destroys the wrong thing.
The test that pinned the aliases is inverted rather than deleted: an alias is
one attribute and comes back by reflex, so a bare
deletethat parses shouldfail a test.
download's "legacy"A doc change only.
--source publicwas documented as "the legacyassetdelivery.roblox.comendpoint". The word was about Roblox's older host,but on the default backend it reads as a deprecation notice. It is the
default precisely because it reaches more assets than Open Cloud does, and
neither is going away.
What is deliberately not here
rbxplace.tomlgets no version key and no lockfile, and the lockfileversion-and-refusal machinery in
rbx-core/src/lockfile.rsstays exactly as itis despite its migration registries being empty.
Those registries are empty on purpose, and the asymmetry is the point:
because it runs in the version already installed. A build that meets a
version it does not understand and reads it anyway drops state in silence.
rbxplace.tomlis the other kind of file: hand-written, in git, with comments.Its version check is per-key rather than per-file (
unknown_keyswarns andnames the key being ignored), which is both more precise and the only thing
that works when one developer upgrades before their teammates. A regenerable
file gets a number and a refusal; a hand-authored one gets detection and a
warning.