Skip to content

doc(marshal): adds missing comment from #2799 review#2801

Merged
erights merged 1 commit into
masterfrom
markm-missing-comment-from-299-review
May 15, 2025
Merged

doc(marshal): adds missing comment from #2799 review#2801
erights merged 1 commit into
masterfrom
markm-missing-comment-from-299-review

Conversation

@erights

@erights erights commented May 15, 2025

Copy link
Copy Markdown
Contributor

Closes: #XXXX
Refs: #2799 (comment)

Description

At #2799 (comment) @dckc suggested

I suggest including it in the comment.

I intended to. But somehow I resolved it before actually doing this, causing me to miss it when evaluating if I should just merge-and-squash #2799. This PR corrects that omission.

Minor internal comment only. It should not have any other implications.

@erights erights self-assigned this May 15, 2025
@erights
erights requested a review from dckc May 15, 2025 21:04
@erights
erights marked this pull request as ready for review May 15, 2025 21:08
@erights
erights force-pushed the markm-missing-comment-from-299-review branch from ef73c72 to 25a283f Compare May 15, 2025 21:09
@mhofman

mhofman commented May 15, 2025

Copy link
Copy Markdown
Contributor

Curious about your thoughts on #2799 (comment) as well if you're following up.

@erights

erights commented May 15, 2025

Copy link
Copy Markdown
Contributor Author

Curious about your thoughts on #2799 (comment) as well if you're following up.

At #2799 (comment)

That's a good idea. I actually tried to do something similar and gave up because, for some reason, it did not occur to me to modify bestEffortStringify itself. Without mod, I noticed that it looked for a Symbol.toString-named property, and I tried to shoehorn the behavior into a getter for that property. Doing this with a simple non-accessor toString seems good.

If I do it, it'll be in PR separate from #2801 . Or, if anyone else wants to try, I'll be happy to review.

@erights
erights merged commit 2e68cc9 into master May 15, 2025
@erights
erights deleted the markm-missing-comment-from-299-review branch May 15, 2025 23:07
boneskull pushed a commit that referenced this pull request Jun 4, 2025
Closes: #XXXX
Refs: #2799 (comment)

## Description

At #2799 (comment) @dckc
suggested
>  I suggest including it in the comment.

I intended to. But somehow I resolved it before actually doing this,
causing me to miss it when evaluating if I should just merge-and-squash
#2799. This PR corrects that omission.

Minor internal comment only. It should not have any other implications.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants