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.

Sign up for GitHub

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

Jump to bottom

BREAKING(asserts): move std/testing/asserts to std/assert #3445

Merged
merged 21 commits into from
Jul 13, 2023
Merged

BREAKING(asserts): move std/testing/asserts to std/assert #3445

merged 21 commits into from
Jul 13, 2023

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Jun 13, 2023 •
edited
Loading

This change moves the functions in std/testing/asserts.ts to their separate files in a new top-level std/asserts folder. Use of _utils/asserts.ts has also been replaced by asserts/asserts.ts. The reasoning behind this is:

  1. Assertions can be called in both production and testing code.
  2. It's at a sufficient size that it deserves its own submodule.
  3. Currently, importing any number of assertion functions also imports all other assertions, bloating dependency imports sometimes. E.g. Deno KV OAuth uses a custom assert() for this reason - to keep dependencies lean.

Related previous migrations:

@iuioiua iuioiua changed the title BREAKING: move std/testing/asserts to std/asserts BREAKING(asserts): move std/testing/asserts to std/asserts Jun 13, 2023
@iuioiua iuioiua marked this pull request as ready for review June 13, 2023 07:19
@iuioiua iuioiua requested a review from kt3k as a code owner June 13, 2023 07:19
@kt3k kt3k requested a review from ry June 13, 2023 08:00
@kt3k
Copy link
Member

kt3k commented Jun 14, 2023

I think this structure make more sense because there're cases when the users call assertions from non test code (for runtime assertion).

Single-export file structure also makes sense to me. If the user wants to use them in runtime code, they should want more granular import instead of importing the entire asserts module.

What do you think? @ry @bartlomieju @piscisaureus @lucacasonato

kt3k
kt3k reviewed Jun 14, 2023
View reviewed changes
testing/asserts.ts Outdated
@@ -1,6 +1,9 @@
// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.

/** A library of assertion functions.
/**
* @deprecated (will be removed after 0.200.0) Import from `std/asserts` instead.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's wait for a little more than usual deprecation period? testing/asserts are heavily depended by the ecosystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The usual deprecation period is 3 minor versions. This is 9 minor versions, like the recent std/semver migration (#3385). Happy to increase this if you still think it should be pushed out.

ry
ry reviewed Jun 14, 2023
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

I'm ok with the move, but I think we should maintain backwards compatibility.

@iuioiua
Copy link
Contributor Author

iuioiua commented Jun 14, 2023 •
edited
Loading

I'm ok with the move, but I think we should maintain backwards compatibility.

Assuming the usual release cadence of a minor release every two weeks, the deprecation would be in place for over 4 months before removal. Backwards compatibility would remain until then.

@kt3k
Copy link
Member

kt3k commented Jun 14, 2023

@ry

I think we should maintain backwards compatibility.

All public exports are kept with @deprecated jsdoc tag (The lsp user will see the strike-through line on deprecated API names)

@iuioiua
The minor release of std usually happens every week (though it's sometimes skipped)

@bartlomieju
Copy link
Member

bartlomieju commented Jun 15, 2023 •
edited
Loading

I would suggest we give it at least 3 months time before removing std/testing/asserts. Other than that, I'm fine with the change.

@iuioiua
Copy link
Contributor Author

iuioiua commented Jun 16, 2023

I've moved the assertion test and extended the removal version to 0.204.0. That's ~3 months from now, assuming a weekly release cadence.

@iuioiua
Copy link
Contributor Author

iuioiua commented Jun 16, 2023

Do we want to replace the use of _util/asserts.ts with std/asserts? I'm for it.

@iuioiua iuioiua requested a review from crowlKats as a code owner June 27, 2023 07:04
@iuioiua iuioiua requested a review from kt3k June 27, 2023 07:11
@iuioiua iuioiua marked this pull request as draft July 4, 2023 01:22
@iuioiua iuioiua marked this pull request as ready for review July 4, 2023 02:08
@iuioiua iuioiua changed the title BREAKING(asserts): move std/testing/asserts to std/asserts BREAKING(asserts): move std/testing/asserts to std/assert Jul 11, 2023
kt3k
kt3k reviewed Jul 11, 2023
View reviewed changes
testing/asserts.ts Outdated Show resolved Hide resolved
kt3k
kt3k approved these changes Jul 13, 2023
View reviewed changes
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 239e85a into denoland:main Jul 13, 2023
@iuioiua iuioiua deleted the asserts branch July 13, 2023 07:10
@boywithkeyboard boywithkeyboard mentioned this pull request Jul 14, 2023
Closed
@fbaltor
Copy link

fbaltor commented Jul 18, 2023

Hey, I don't know if this is the right place to ask, but from my understanding we must change the asserts being used in the deno as well, right?

image

@bartlomieju
Copy link
Member

@fbaltor it's very strange that you get these warnings - in deno repository we have a git submodule for deno_std (https://github.com/denoland/deno_std/tree/b23a76a47adaf63b5493761a87e6dd0dc0de0bb8) which hasn't been updated to latest version yet.

@fbaltor
Copy link

fbaltor commented Jul 18, 2023

You are absolutely right. I made some confusion by inadvertently updating the submodules. Thank you, @bartlomieju!

@iuioiua iuioiua mentioned this pull request Jul 20, 2023
Merged
kwj added a commit to kwj/project-euler that referenced this pull request Jul 20, 2023
@kwj
Move std/testing/asserts to std/assert
02dcc71 
ref: denoland/std#3445
@andrinheusser andrinheusser mentioned this pull request Aug 7, 2023
Merged
yoshida-ryuhei added a commit to yoshida-ryuhei/ddu-source-vim-lsp that referenced this pull request Aug 17, 2023
@yoshida-ryuhei
Change assert to follow denoland/std#3445
5124179
yoshida-ryuhei added a commit to yoshida-ryuhei/deno_blog that referenced this pull request Feb 5, 2024
@yoshida-ryuhei
move std/testing/asserts to std/assert
db7a1e9 
Reference: denoland/std#3445
yoshida-ryuhei added a commit to yoshida-ryuhei/deno_blog that referenced this pull request Feb 5, 2024
@yoshida-ryuhei
move std/testing/asserts to std/assert
3ca39d0 
Reference: denoland/std#3445
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry left review comments

@kt3k kt3k kt3k approved these changes

@crowlKats crowlKats Awaiting requested review from crowlKats crowlKats is a code owner

Assignees
No one assigned
Labels
archive async bytes collections console crypto csv datetime dotenv encoding examples flags fmt front-matter fs html http io json jsonc log media-types path regexp semver signal streams testing toml uuid wasi yaml
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@iuioiua @kt3k @bartlomieju @piscisaureus @fbaltor @ry