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

doc: add esm examples for assert #37607

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 4, 2021

ESM example variants in preparation for #37162

/cc @aduh95

Signed-off-by: James M Snell jasnell@gmail.com

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Mar 4, 2021
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I have a preference for keeping the default import for assert (assert.throws, assert.ok, assert.fail, etc.), but not blocking.

doc/api/assert.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Mar 5, 2021

@nodejs/documentation @nodejs/assert @nodejs/modules

@Trott
Copy link
Member

Trott commented Mar 5, 2021

I hesitate to say this because I know doing this was a lot of work to begin with, but I'm with @aduh95 on the named imports. I'd prefer we do default exports because assert.throws() is a lot more clear than throws() if someone is searching through the documentation. If we want to switch to named imports/destructuring, that could be a separate PR. It would have the benefit of reducing the churn in this too.

(Full disclosure: I happen to think destructuring/named imports are a bit of an anti-pattern. If I have two modules with a get() function, it sure is a lot more clear if I'm using http.get() and https.get() at invocation rather than just get() and having to go to the require/import statement to figure out which one I'm using. So on the one hand, this is just my personal preference. But on the other hand, our example code is going to become other people's production code, so since I happen to believe that one way of doing it is actually generally better, I'd prefer we do the better thing in our example code.)

@jasnell
Copy link
Member Author

jasnell commented Mar 5, 2021

What?! Lol... I'm not worried about the work involved, it's really not that much.

@aduh95 aduh95 mentioned this pull request Mar 5, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Mar 5, 2021

There, I fixed it for you picky folk 😁🤣

@ljharb
Copy link
Member

ljharb commented Mar 5, 2021

altho tbh the AssertionError case is the one where named imports seem clearer to me :-p fine as-is ofc!

@jasnell
Copy link
Member Author

jasnell commented Mar 5, 2021

image

;-)

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 5, 2021
@jasnell
Copy link
Member Author

jasnell commented Mar 8, 2021

Landed in a8b5cdc

@jasnell jasnell closed this Mar 8, 2021
jasnell added a commit that referenced this pull request Mar 8, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37607
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 16, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37607
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants