Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Checker" parameters would be more ergonomic as "Rejectors" #2285

Open
gibson042 opened this issue May 15, 2024 · 6 comments
Open

"Checker" parameters would be more ergonomic as "Rejectors" #2285

gibson042 opened this issue May 15, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@gibson042
Copy link
Contributor

gibson042 commented May 15, 2024

What is the Problem Being Solved?

#2284 demonstrated that there is a relatively small but still significant performance cost to creating reject = !!check && ((T, ...subs) => check(false, X(T, ...subs))) functions that will only be used in failure paths, and so moved that definition into a new CX function and updated many sites to use that:

-    reject && reject`…`
+    !!check && CX(check)`…`

However, this has drawbacks of its own with respect to comprehensibility and readability, especially when prettier introduces line breaks like

    (
      !!check &&
      CX(
        check,
      )`…`
    )

Description of the Design

I think we'd be better off replacing (cond: boolean, details?: Details | undefined) => boolean "Checker" parameters with optional (template: TemplateStringsArray | string[], ...args: any[]) => false "Rejector" parameters, recovering the old local pattern but without local function creation.

And per #2285 (comment) , we'll also want to rename internal functions for indicating the progress of this transition.

-const checkSomething = (candidate, check = undefined) => {
+const confirmSomething = (candidate, reject = false) => {
   return (
     isOk(candidate) ||
-    (!!check && CX(check)`…`)
+    (reject && reject`…`)
   );
 };

And at the highest possible level, we'd replace use of x => x checkers with undefined rejectors and assertChecker with assert.Fail.

-    checkSomething(candidate, assertChecker)
+    confirmSomething(candidate, Fail)

Security Considerations

None that I can see.

Scaling Considerations

Negligible, but the new pattern might be slightly faster.

Test Plan

n/a, this is behavior-preserving refactoring.

Compatibility Considerations

assertChecker is exported by the pass-style package, and used by at least the patterns package, which also makes use of checkers so should be updated similarly. But I think all such use is internal for supporting {is,assert}$Type package exports (e.g., isKey and assertKey).

Upgrade Considerations

If any exports do expose a Checker parameter, we'll need to consider a deprecation strategy.

@gibson042 gibson042 added the enhancement New feature or request label May 15, 2024
@erights
Copy link
Contributor

erights commented May 15, 2024

I like this a lot! Deprecation is somewhat of an issue. Fortunately, most packages only use a checkFoo internally, exporting only the isFoo and assertFoo callers, which would not need to change. To help preserve correctness across this transition, I suggesting changing the checkFoo names to something else. What?

@gibson042
Copy link
Contributor Author

Hmm, I was thinking the "check" names would remain, but that does make sense. The "is" names would be perfect if they weren't theirselves exposed, but I'd rather not reveal this new pattern (at least not right now). So I guess we're looking at synonyms or near-synonyms of "check" ("verify", "ensure", "test", "matches", etc.). What about "matches", in loose correspondence with patterns?

export const isSafePromise = pr => matchesSafePromise(pr);
export const assertSafePromise = pr => matchesSafePromise(pr, Fail);

I think that would work directly everywhere except in patternMatchers.js, where we'd probably redefine both checkMatches and matches in terms of e.g. const privateMatches = (specimen, pattern, reject = false, label = undefined) => ….

@erights
Copy link
Contributor

erights commented May 16, 2024

I like the direction. I'm ok with the choice of matchesFoo but the collision you mention does make it awkward. You mention test. How about testFoo? I think I like that better. Does it have any problematic collisions?

@erights
Copy link
Contributor

erights commented May 16, 2024

After more reflection, I really don't like matchesFoo because it sounds too specific to patterns and pattern matching. I also no longer like testFoo because it sounds like it has something to do with our ava regression testing. ensureFoo isn't quite right, because it suggests that it makes sure that foo is true. verifyFoo isn't bad. What are other candidates?

@gibson042
Copy link
Contributor Author

gibson042 commented May 16, 2024

https://www.merriam-webster.com/thesaurus/verify suggests "confirm", "validate", "certify", "affirm", etc. Of the entire list, I think I like "verify" and "confirm" best.

export const isSafePromise = pr => verifySafePromise(pr);
export const assertSafePromise = pr => verifySafePromise(pr, Fail);
export const isSafePromise = pr => confirmSafePromise(pr);
export const assertSafePromise = pr => confirmSafePromise(pr, Fail);

@erights
Copy link
Contributor

erights commented May 16, 2024

Both sound good to me. I like confirmFoo slightly better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants