Skip to content

Conversation

@MichaelDeBoey
Copy link
Member

@MichaelDeBoey MichaelDeBoey commented Nov 2, 2024

I've put the non-exported classes in here as well in case we would ever need them

Probably closes #377


Out of scope for this PR as the class isn't exported, but I was wondering how we would handle the "extends node:stream's Readable class" in the case of TestsStream 🤔

TestsStream is only created in 16.19.0 & 18.9.0, but node:stream's Readable class is already existing way earlier.
For functionality that's present in node:stream's Readable class before 16.19.0 & 18.9.0 we should just use 16.19.0 & 18.9.0 in TestsStream I think?
And for functionality that's added to node:stream's Readable class after 16.19.0 & 18.9.0, we just keep the same values I think?

Since I wasn't sure, I kept the original value as a comment

Unfortunately this means that we would need to find some way of noticing ourselves that we need to update TestsStream whenever we update node:stream's Readable class

We will have the same problem with TestContext's assert property and all top-level functions from node:assert

@MichaelDeBoey MichaelDeBoey added the rule:update An update to a current rule label Nov 2, 2024
@MichaelDeBoey MichaelDeBoey requested review from a team and scagood November 2, 2024 02:54
@MichaelDeBoey MichaelDeBoey self-assigned this Nov 2, 2024
@MichaelDeBoey MichaelDeBoey force-pushed the extract-test-globals-correctly branch 2 times, most recently from 8b88f6e to df7f0bd Compare November 2, 2024 03:06
Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

A few initial thoughts 🤔, generally looks good!

@MichaelDeBoey MichaelDeBoey force-pushed the extract-test-globals-correctly branch from df7f0bd to 0d81ddc Compare November 4, 2024 14:33
@MichaelDeBoey MichaelDeBoey requested a review from scagood November 4, 2024 15:29
Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

Do you want to remove the excess comments before merging?

@MichaelDeBoey
Copy link
Member Author

Do you want to remove the excess comments before merging?

@scagood You mean the types that aren't used?

As I said I my OP, I kept them for future reference if we ever need them

@scagood scagood merged commit 0b228dd into master Nov 5, 2024
31 checks passed
@scagood scagood deleted the extract-test-globals-correctly branch November 5, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rule:update An update to a current rule

2 participants