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

refactor: use _util/asserts.ts for non-test code #2857

Merged
merged 5 commits into from
Nov 10, 2022
Merged

refactor: use _util/asserts.ts for non-test code #2857

merged 5 commits into from
Nov 10, 2022

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Nov 8, 2022

This PR:

  1. More appropriately renames _util/assert.ts to _util/asserts.ts.
  2. Adds missing unreachable assertion to _util/asserts.ts, along with corresponding test in _util/asserts_test.ts.
  3. Corrects non-test code within the codebase that previously used testing/asserts.ts to use _util/asserts.ts.
  4. Adds a small explainer to _util/assert.ts and the "Contributing" section of the repo readme.

This is half of what was discussed in #2849. I'll create a tool to enforce the rule after this PR lands.

@iuioiua iuioiua changed the title refactor: ensure _util/asserts.ts is used for non-test code refactor: use _util/asserts.ts for non-test code Nov 8, 2022
@iuioiua iuioiua marked this pull request as ready for review November 8, 2022 22:33
@kt3k
Copy link
Member

kt3k commented Nov 9, 2022

I think what we actually need in _util/asserts.ts are only assert and unreachable and those don't need to depend on testing/asserts.ts.

@iuioiua
Copy link
Contributor Author

iuioiua commented Nov 9, 2022

Thanks for the good suggestions! I've made the changes, except for moving assertCallbackErrorUncaught into node/_test_util.ts, as I don't believe a whole file should exist for a single function. Perhaps, it can be revisited in another PR.

I've redone my initial comment on this PR to reflect the new changes.

Comment on lines +115 to +118
All internal non-test code, that is files that do not have `test` or `bench` in
the name, must use the assertion functions within `_utils/asserts.ts` and not
`testing/asserts.ts`. This is to create a separation of concerns between
internal and testing assertions.
Copy link
Member

Choose a reason for hiding this comment

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

Nice. Thanks!

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@kt3k kt3k merged commit 792f133 into denoland:main Nov 10, 2022
@iuioiua iuioiua deleted the util-asserts branch November 10, 2022 07:28
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.

2 participants