RemoveOptions: backtick the Default<FieldName> member reference - #612
Merged
Conversation
Same class of bug as the record-construction fix, at a different site. [<ArgumentDefaultFunction>] makes the generator concatenate "Default" with the field's name and emit the result as an identifier -- twice, once to set the default and once to render it into help text. A field whose name needs backticks forces the member to be declared with them, and the reconstruction dropped them, so the generated file did not parse. The concatenation cuts both ways and the fix accounts for both: a field named ``mod`` needs backticks itself, but the member `Defaultmod` does not, and is still emitted bare. isValidBareRecordLabel/backtickRecordLabel are renamed to isValidBareIdent/backtickIdent, since they now serve a member name as well as a record label. The probe deliberately stays in the record-label position for every caller: it is the tightest one available, admitting exactly one identifier and nothing else. A probe in the position this new caller emits into would be far weaker -- `Owner.Defaultspace name ()` parses happily as an application, and would report success for a name which is nothing of the sort. Test first, observed failing: ConsumePlugin gained a record whose default-function fields have awkward names, and the build broke with `AwkwardDefaultFunctionName.Defaultspace in name()` before the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same bug as the previous commit fixed in ArgParserGenerator, in a
different generator: `Option.defaultWith Type.Default<FieldName>` builds
that member name by concatenation and emits it as a synthesized Ident, so
a field whose name needs backticks produced
input.``d thing`` |> Option.defaultWith RecordType.Defaultd thing
Only that one name was affected. The field declaration, the accessor and
the record label all reuse the Ident from the user's source, which has a
range Fantomas can slice the original text from, so they were already
spelled correctly -- confirmed by adding a required awkwardly-named field
alongside the optional one and watching only the Default reference break.
The backticking logic moves out of ArgParserGenerator into AstHelper.fs
as `BacktickIdent`, since two generators now need it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # WoofWare.Myriad.Plugins/ArgParserGenerator.fs
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
1 participant
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.
Stacked on #611 — the same bug in a second generator, and they now share the fix.
RemoveOptionsGeneratorbuildsOption.defaultWith Type.Default<FieldName>by concatenation and emits the result as a synthesizedIdent, so a field whose name needs backticks produced:Only that one name was affected, and I checked rather than assuming: the field declaration, the accessor and the record label all reuse the
Identfrom the user's own source, which carries a range Fantomas can slice the original text from. To pin that down,ConsumePlugin'sRecordTypegained a required awkwardly-named field alongside the optional one — and only theDefaultreference broke, with the other three sites already correct. Both fields stay in the schema so the distinction keeps being exercised.The backticking logic moves out of
ArgParserGeneratorintoAstHelper.fsasBacktickIdent(isValidBare/escape), since two generators now need it. That's a pure move plus the rename; the reasoning about why the probe uses the record-label position for every caller moves with it.The existing
shortenPropertyFsCheck property is extended rather than a new example test added, so the new fields are exercised across arbitrary inputs, including both theNone(defaulted) andSomebranches.Suite green (1105 / 164 / 3).
🤖 Generated with Claude Code