Skip to content

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

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

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

Conversation

@erights
Copy link
Copy Markdown
Contributor

@erights erights commented May 15, 2025 •
edited
Loading

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
@erights erights mentioned this pull request May 15, 2025
Merged
2 tasks
@mhofman
Copy link
Copy Markdown
Contributor

mhofman commented May 15, 2025

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

@erights
Copy link
Copy Markdown
Contributor Author

erights commented May 15, 2025

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
16 checks passed
@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
@erights @boneskull
doc(marshal): adds missing comment from #2799 review (#2801)
3947349 
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

Reviewers

@dckc dckc dckc approved these changes

Assignees

@erights erights

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erights @mhofman @dckc