Repository navigation
Model constructors report arguments they do not use - #622
Merged
Merged
Conversation
Every exported model constructor now warns about a name in `...` that reaches no consumer, instead of dropping it. The names each constructor passes on to its .model_*() internal (links, default_priors, IMMabc's nt_distances) are matched by prefix, as R matches them there, and the deprecated setsize exactly; unnamed extras are reported too. A close misspelling gets a "Did you mean" line. The warning says it becomes an error in bmm 1.6.0. A convention test finds the constructors in the namespace (exports with `...` that call a .model_*() internal) and requires the check in each.
…ors (#552) The constructors' check now matches R's argument matching when an exact name and a prefix of the same argument appear in one call: `links = ` together with `l = ` reports `l`, which R drops. A function object as the call head (`do.call(sdm, ...)`, `Map(sdm, ...)`, `lapply(x, sdm, ...)`) no longer prints the function's source; the warning says "this model constructor" and gives no help page. "Did you mean" needs a name close in proportion to its length, so `xy` no longer points to `m`, and never offers an argument the call already supplied. The NEWS entry now also covers an extra argument given by position. Tests: the setsize exemption is tested for all five constructors that read it (mixture3p, imm, IMMfull, IMMbsc, IMMabc), with the exact warnings. The convention test now compares the `known` argument at each call site with the free formals of the .model_*() internal the constructor calls, and requires `known` and `deprecated` to be character literals. Added tests for prefix matching, suggestions and function-object call heads.
…constructor check (#552) A misspelling is now compared within half its length, at most two edits, so a transposition in a four or five letter name such as `ilnks` for `links` or `idst` for `dist` still gets a "Did you mean" hint, while a two-letter name does not point to a one-letter argument. An argument given twice is reported once. The tests can no longer pass on an empty result. Calls are evaluated without swallowing errors, except in the few that stop after the warning (a required argument is missing, or the call head is a function object), and each suggestion test compares the whole message. New tests pin the length rule at both ends, the exclusion of arguments the call already gave, by exact name or by prefix, and the count and plural of unnamed arguments.
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.
Closes #552 (item 5, the last open one).
Every exported model constructor used to drop an argument it did not know:
mpt(trees, foo = 1),sdm("y", foo = 1)and the other 16 returned a model without a word. Each now calls.check_constructor_dots()right aftermatch.call(), which warns, names every unused argument and the constructor, and points to its help page. For a misspelling within two edits of a real argument it adds "Did you meanlinks?".What still passes through
..., per constructor:imm(),IMMfull(),IMMbsc()linkssetsizeIMMabc()links,nt_distancessetsizemixture3p()linkssetsizemixture2p(),sdm(),sdmSimple()linksm3()links,default_priorsmpt()default_priorscswald(),ddm(),ezdm(), the fivesdt_*()Passed-on names are matched the way R matches them in the internal, by unique prefix:
sdm("y", link = ...)reacheslinkstoday and is not reported. Unnamed extras are reported too; insdm()the third one went intolinksby position.D1, warning or error: needs your decision. This PR warns, and the message says it becomes an error in bmm 1.6.0. A script with a stray argument has run until now, which fits rule 1 of the lifecycle policy. Erroring now is a one-word change.
D2. Partial matching of a constructor's own formals (
sdm("y", vers = "simple")) is left as it is.A convention test finds the constructors in the namespace (exports with
...that call a.model_*()internal) and fails for any without the check. Once develop is merged into them it fails on #500, #501, #502, #507, #524 and #591-#593; the fix there is one line per constructor. Measured on #524.