Skip to content

Update Value.spec.lua#165

Merged
dphfox merged 1 commit into
dphfox:mainfrom
Dionysusnu:patch-6
Feb 1, 2023
Merged

Update Value.spec.lua#165
dphfox merged 1 commit into
dphfox:mainfrom
Dionysusnu:patch-6

Conversation

@Dionysusnu

Copy link
Copy Markdown
Contributor

No description provided.

@dphfox

dphfox commented Jun 4, 2022

Copy link
Copy Markdown
Owner

What's the motivation for this change?

@dphfox

dphfox commented Feb 1, 2023

Copy link
Copy Markdown
Owner

This change does not seem meaningful. Returning to it now I do not see any reason why this should be at all functionally different, and since no explanation has been given, I have no choice but to reject this PR.

@Dionysusnu

Copy link
Copy Markdown
Contributor Author

and since no explanation has been given, I have no choice but to reject this PR.

this was explained several times, but ignored on each. https://discord.com/channels/385151591524597761/895437663040077834/982713104574079037
https://discord.com/channels/385151591524597761/895437663040077834/980596243317272596

And a very simple look at the code would have shown it too. Passing a state directly into a Computed is not a valid way to use it. The test is clearly written to use a ForValues to test the garbage collection behaviour.

@dphfox

dphfox commented Feb 1, 2023

Copy link
Copy Markdown
Owner

and since no explanation has been given, I have no choice but to reject this PR.

this was explained several times, but ignored on each. https://discord.com/channels/385151591524597761/895437663040077834/982713104574079037
https://discord.com/channels/385151591524597761/895437663040077834/980596243317272596

And a very simple look at the code would have shown it too. Passing a state directly into a Computed is not a valid way to use it. The test is clearly written to use a ForValues to test the garbage collection behaviour.

Right so this is why I would have preferred the explanation to be here rather than buried in Discord chat. It is impossible to keep track of a pull requests history like that; while I was going through old PRs last night there was no explanation attached here and I had no idea of your intention whatsoever. Don't trust me to remember, please put the information where I (and perhaps more importantly, others) can find it.

I will look at this again later when I'm done with work.

@dphfox dphfox reopened this Feb 1, 2023
@dphfox dphfox added not ready - evaluating Currently gauging feedback and removed status: rejected labels Feb 1, 2023
@dphfox dphfox merged commit 14b5288 into dphfox:main Feb 1, 2023
@dphfox

dphfox commented Feb 1, 2023

Copy link
Copy Markdown
Owner

Apologies for the former lapse in judgement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

not ready - evaluating Currently gauging feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants