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

impl abstract-equality-comparison #395

Merged
merged 11 commits into from
May 13, 2020
Merged

impl abstract-equality-comparison #395

merged 11 commits into from
May 13, 2020

Conversation

hello2dj
Copy link
Contributor

@hello2dj hello2dj commented May 12, 2020 •
edited
Loading

This Pull Request fixes/closes jasonwilliams#351 jasonwilliams#357

It changes the following:

  • impl abstract-equality-comparison recursive version except BigInt & NaN
  • add tests
  • remove PartialEq on Value
  • same_value & same_value_zero impl

HalidOdat
HalidOdat reviewed May 12, 2020
View reviewed changes
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good! Maybe check my comments on how we can improve it.

boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat mentioned this pull request May 12, 2020
Merged
@Razican Razican added the enhancement New feature or request label May 12, 2020
@Razican Razican added this to the v0.8.0 milestone May 12, 2020
@Razican Razican linked an issue May 12, 2020 that may be closed by this pull request
"-0" == 0 and "+0" == 0 returns false, it should return true #357
Closed
@Razican @HalidOdat
Add [] == ![]test
b138edf 
Co-authored-by: HalidOdat <halidodat@gmail.com>
Razican
Razican reviewed May 12, 2020
View reviewed changes
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This looks very good, I added some documentation as suggestions.

boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
hello2dj and others added 3 commits May 12, 2020 18:16
@hello2dj @Razican
Update boa/src/builtins/value/operations.rs
ad5be3a 
Co-authored-by: Iban Eguia <razican@protonmail.ch>
@hello2dj @Razican
Update boa/src/builtins/value/operations.rs
c25445c 
Co-authored-by: Iban Eguia <razican@protonmail.ch>
fix(value): move tests to tests.rs
a6a63e2
Razican
Razican reviewed May 12, 2020
View reviewed changes
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

I think this is almost ready, give a look to my comments :)

boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
@hello2dj hello2dj mentioned this pull request May 13, 2020
Closed
@Razican Razican linked an issue May 13, 2020 that may be closed by this pull request
same_value is not spec compliant #394
Closed
Razican
Razican reviewed May 13, 2020
View reviewed changes
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This is looking very good, it's clearly spec compliant! Congrats, give a look to my comments to see if there are improvements to make, and I think it's almost ready for merging :)

boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
boa/src/builtins/value/operations.rs Show resolved Hide resolved
Razican
Razican reviewed May 13, 2020
View reviewed changes
boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/number/mod.rs Show resolved Hide resolved
boa/src/builtins/number/mod.rs Show resolved Hide resolved
boa/src/builtins/number/mod.rs Show resolved Hide resolved
boa/src/builtins/value/operations.rs Outdated Show resolved Hide resolved
dengjie and others added 3 commits May 13, 2020 17:31
@hello2dj hello2dj requested a review from Razican May 13, 2020 13:24
Razican
Razican approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This looks very good! congrats, it's ready for merge from my side :)

HalidOdat
HalidOdat approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good to me too!

jasonwilliams
jasonwilliams approved these changes May 13, 2020
View reviewed changes
Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

Looks good to me three

@HalidOdat HalidOdat changed the title impl abstract-equality-comparison impl abstract-equality-comparison May 13, 2020
@HalidOdat HalidOdat merged commit 402041d into boa-dev:master May 13, 2020
JavedNissar pushed a commit to JavedNissar/boa that referenced this pull request May 14, 2020
@hello2dj @JavedNissar
impl abstract-equality-comparison (boa-dev#395)
bc5f087
@Razican Razican mentioned this pull request May 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Razican Razican Razican approved these changes

@jasonwilliams jasonwilliams jasonwilliams approved these changes

@HalidOdat HalidOdat HalidOdat approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.8.0
4 participants
@hello2dj @Razican @jasonwilliams @HalidOdat