Fix the deploy regression strict parsing shipped: --beam-flags no longer works - #67
Merged
Merged
Conversation
…ger works A post-hoc review of #63 — which I merged without one — found that switching `mix mob.deploy` to strict parsing broke the documented invocation. `OptionParser` will not consume a dash-prefixed argument as a `:string` value, so `--beam-flags "-S 4:4 -A 4"` parses as two unknown options. BEAM flags essentially all begin with a dash, so this is not an edge case: it is every spelling this repo prints, in seven places — the task's own moduledoc, five README recipes, and the "untuned variant" step of both battery benchmarks. Under the old lenient parsing the value was silently dropped and the deploy carried on with whatever `mob.exs` held, which is its own bug. Strict parsing turned that into a hard abort whose message named `--beam-flags` — a valid option — as unknown. So the fix for one silent failure produced a loud wrong one, and shipped because I skipped the review on a change I had filed mentally as small. `--flag value` is now rewritten to `--flag=value` when the value starts with a dash, for string-valued switches only. Doing it for every switch would swallow the flag after a boolean. Three more from the same review: The error conflated "unrecognised flag" with "recognised flag, unparseable value" and discarded the value, so `--schedulers abc` reported `--schedulers` as unknown and sent the reader to a help page that lists it. `mob.new_plugin` already handled this correctly; this now matches. Positional arguments were still silently dropped. `mix mob.deploy --native ABC123` — a natural fumble of `--device` — parsed cleanly and deployed to every device, which is the identical symptom to the bug #63 set out to fix, in the same task, left in place by it. The four tests #63 shipped were all source-text assertions and none ran the parser, so none could have caught any of this. There is now a table test over every invocation the docs actually print, which fails if a switch is renamed or a documented spelling stops working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
This is a live regression on master. A post-hoc adversarial review of #63 — which I merged without one — found that switching
mix mob.deployto strict parsing broke the documented invocation.OptionParserwill not consume a dash-prefixed argument as a:stringvalue, so:BEAM flags essentially all begin with a dash, so this isn't an edge case — it's every spelling this repo prints, in seven places: the task's own moduledoc, five README recipes, and the "untuned variant" step of both battery benchmarks. The message compounds it by naming
--beam-flags, a valid option, as unknown.Under the old lenient parsing the value was silently dropped and the deploy carried on with whatever
mob.exsheld — its own bug. Strict parsing turned that into a loud wrong one.--flag valueis now rewritten to--flag=valuewhen the value starts with a dash, for string-valued switches only (doing it for every switch would swallow the flag after a boolean).Three more from the same review
--schedulers abcreported--schedulersas unknown and sent the reader to a help page listing it.mob.new_pluginalready had this right; this now matches.mix mob.deploy --native ABC123parsed cleanly and deployed to every device — the identical symptom to the bug Refuse unknown options in mob.deploy instead of ignoring them #63 set out to fix, in the same task, left in place by it.CHANGELOG gains a
### Changedentry for the breaking half: anyone with a wrapper script passing an extra flag goes from working to exit 1, and #63 didn't say so.2331 tests, credo and format clean.