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.

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

Use to primitive for plus operation and date (taken from #1611 done by @tonygermano) #1685

Merged
merged 6 commits into from
Oct 11, 2024

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Oct 7, 2024

This is another carve out from #1661.

Please squash this on merge.

@rbri rbri marked this pull request as draft October 7, 2024 16:13
@rbri rbri marked this pull request as ready for review October 9, 2024 13:06
@rbri
Copy link
Collaborator Author

rbri commented Oct 9, 2024

From my point of view this is ready to be squash-merged... what do you think?

throw ScriptRuntime.typeErrorById(
"msg.invalid.toprimitive.hint", ScriptRuntime.toString(arg0));
}
;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor, but do you know why there is a semicolon here?

prototype/Symbol.toPrimitive/hint-string-first-valid.js
prototype/Symbol.toPrimitive/length.js
prototype/Symbol.toPrimitive/name.js
prototype/Symbol.toPrimitive/called-as-function.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we made a bunch of new tests pass, but we also made a test that previously passed fail, from what it looks like. Do you know why this appears to be failing now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have seen this alteady but there was no time for that. Will check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

called-as-function.js seems to be a more general problem we have the same with the toJSON prototype.
This function passes in the past because a type error was expected (and there was no toPrimitiveSupport).

Will have another look at this and maybe open an issue for that, but in gerneral it is nothing introduced by this one.

@gbrail
Copy link
Collaborator

gbrail commented Oct 10, 2024

Yes, it looks good, but I have two minor points -- can you check them out? Thanks!

@gbrail
Copy link
Collaborator

gbrail commented Oct 11, 2024

OK -- thanks for looking at the test and removing that huge problem with the semicolon ;-)

@gbrail gbrail merged commit 66622e5 into mozilla:master Oct 11, 2024
1 check passed
@rbri rbri deleted the use_toPrimitive_plus_and_date branch October 11, 2024 07:02
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.

2 participants