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

add .destroy to noAutoDestroy iterator #95

Merged
merged 1 commit into from
Feb 6, 2024
Merged

add .destroy to noAutoDestroy iterator #95

merged 1 commit into from
Feb 6, 2024

Conversation

Kelerchian
Copy link
Contributor

@Kelerchian Kelerchian commented Feb 5, 2024 •
edited
Loading

Resolves #94

@Kelerchian Kelerchian linked an issue Feb 5, 2024 that may be closed by this pull request
Add local destroy in MachineRunnerIterableIterator #94
Closed
@Kelerchian Kelerchian force-pushed the ada/94-add-local-destroy-in-machinerunneriterableiterator branch from 853cc1b to e95c038 Compare February 5, 2024 14:41
@Kelerchian Kelerchian requested a review from rkuhn February 5, 2024 14:50
rkuhn
rkuhn reviewed Feb 5, 2024
View reviewed changes
Copy link
Member

@rkuhn rkuhn left a comment

Choose a reason for hiding this comment

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

I don’t have all of it in active working memory: didn’t you mention that the .noAutoDestroy() result had the wrong destruction wired in somehow?

machine-runner/tests/esm/runner.test.ts Outdated
expect(cres2.done).toBeTruthy()
const cres1_2 = await cloned1.peek()
const cres2_2 = await cloned1.peek()
const cres3_2 = await cloned1.peek()
Copy link
Member

Choose a reason for hiding this comment

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

isn’t the point of .noAutoDestroy() (since my changes) that this should also work with .next()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, my bad. those should be cloned1, cloned2, and cloned 3

@Kelerchian Kelerchian force-pushed the ada/94-add-local-destroy-in-machinerunneriterableiterator branch from e95c038 to 42fb74b Compare February 5, 2024 16:03
@Kelerchian
Copy link
Contributor Author

didn’t you mention that the .noAutoDestroy() result had the wrong destruction wired in somehow?

No. The wrong wiring was a bug in user's code.
However, that bug reminds me that missing pieces such as this can easily cause (although indirectly) a semantic misunderstanding.

@Kelerchian Kelerchian force-pushed the ada/94-add-local-destroy-in-machinerunneriterableiterator branch from 42fb74b to 1560aa9 Compare February 6, 2024 08:43
@Kelerchian Kelerchian merged commit 8595842 into master Feb 6, 2024
1 check passed
@Kelerchian Kelerchian deleted the ada/94-add-local-destroy-in-machinerunneriterableiterator branch February 6, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkuhn rkuhn rkuhn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add local destroy in MachineRunnerIterableIterator
2 participants
@Kelerchian @rkuhn