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

'assert' package #1717

Merged
merged 2 commits into from
Aug 14, 2023
Merged

'assert' package #1717

merged 2 commits into from
Aug 14, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Aug 10, 2023

closes: #1703

Description

A new package to provide the properties of the assert global such that they can be auto-imported in an IDE.

Security Considerations

If this dependency were compromised it could lead a user to thinking they're using the SES's assertions when they're not.

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

New tests. Lightly covered yet since the idea is that eventually this package becomes the shim.

Upgrade Considerations

n/a

@turadg turadg requested a review from kriskowal August 10, 2023 22:39
Comment on lines 76 to 77
details as X,
quote as q,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to export the full name and alias at the import site.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My rationale is that abbreviations are for local ergonomics and the exported names need to communicate their purpose. A reviewer unfamiliar with the assert module should be able to infer the meaning from usage and name without leaving the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree they don't belong in Endo. I'll take them out.

I don't necessarily agree the aliasing should happen at the call site. To make imports easy a repo might have a utils package that re-exports them under abbreviations that are normal in the repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Good enough for this change. We can converge on the broader tension between legibility and ergonomics elsewhere.

@turadg turadg merged commit 8bf3a8f into master Aug 14, 2023
13 checks passed
@turadg turadg deleted the 1703-assert-package branch August 14, 2023 22:28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty README. 😭

Comment on lines +70 to +72
// properties with syntax collision
assertString,
assertTypeof,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why rename the exports?

Suggested change
// properties with syntax collision
assertString,
assertTypeof,
assertString as string,
assertTypeof as typeof,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it saves the importer from aliasing them. I'm open to making the change you suggest. I could add a README at that point. Also be my guest if you like to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know how I missed this, and I have no strong feelings one way or the other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, yes, I do feel strongly that all these names should be consistently exported either assertX or x to favor either the import * as assert; assert.x() usage or the import { assert, assertX, assertY } usage.

Copy link
Member

@kriskowal kriskowal Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More broadly, these are the design families:

  1. import { assert, assertTypeof, assertString, assertEtc } from '@endo/assert';
    assert();
    assertTypeof();
    assertString();
    Express your preference with 🎉
  2. import { assert } from '@endo/assert';
    assert();
    assert.typeof();
    assert.string();
    assert.etc();
    Express your preference with ❤️
  3. import * as assert from '@endo/assert';
    assert.ok(); // unprecedented
    assert.string();
    assert.typeof();
    assert.etc();
    Express your preference with 🚀

My suspicion is that we disfavor option 3(🚀) collectively. @erights’s position seems consistent with blessing one of these forms and herding usage. @turadg’s motivation for the creation of this package is having an anchor for import inference which is probably satisfied by any of these forms.

I believe that @erights’s position of having a single consistent form actually implies that option 1(🎉) should preclude 2(❤️) by exporting an assert function that unlike globalThis.assert lacks the assert.xyz() properties.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we resolved that we're doing likewise with
import { quote as q, bare as b, something as X }
with no separate renaming reexport anywhere?

I'm not in love with errorMessage and throwError, though I don't hate them either. But if we're agreed we're pervasively renaming to get to q, b, and X, I have no objection to adding
import { something as Fail }.
If we're all agreed on this much, then lets proceed to bikeshed the two unresolved somethings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mind appears to be entangled at a quantum level with @michaelfig. I prefer to avoid abbreviation in public API and to scope abbreviations lexically when that improves local legibility. I propose redact for X and throwRedact for Fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like redact and throwRedact !!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I like making names longer, but how about throwRedacted?

(and redacted if that seems more consistent to y'all.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sleeping on it, I do like redacted and throwRedacted better.

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

Successfully merging this pull request may close these issues.

provide an import for members of the assert global
7 participants