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

Parser 3.0: methods #2151

Merged
merged 10 commits into from
Jan 21, 2021
Merged

Parser 3.0: methods #2151

merged 10 commits into from
Jan 21, 2021

Conversation

hmdne
Copy link
Member

@hmdne hmdne commented Dec 31, 2020

Itself not related to the parser, but implements a few methods that were added between Ruby 2.6 and 3.0.

@hmdne hmdne mentioned this pull request Jan 2, 2021
hmdne added a commit that referenced this pull request Jan 2, 2021
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

Great work! ✨
Just a question, and the request to squash the rubocop fixes to the original commits.

opal/corelib/comparable.rb Outdated Show resolved Hide resolved
opal/corelib/comparable.rb Outdated Show resolved Hide resolved
opal/corelib/comparable.rb Outdated Show resolved Hide resolved
@hmdne hmdne marked this pull request as draft January 2, 2021 22:31
@hmdne
Copy link
Member Author

hmdne commented Jan 2, 2021

I will need to do a little bit of more work on this. But it seems good enough, of 700 fails added by a version change, this changeset removes 100 of them.

@elia
Copy link
Member

elia commented Jan 3, 2021

I will need to do a little bit of more work on this. But it seems good enough, of 700 fails added by a version change, this changeset removes 100 of them.

@hmdne I'm always ok with splitting if you want to have the part of this that's already ok merged right away

hmdne added a commit to hmdne/opal that referenced this pull request Jan 3, 2021
@hmdne hmdne marked this pull request as ready for review January 3, 2021 22:32
@hmdne
Copy link
Member Author

hmdne commented Jan 3, 2021

@elia If you see a problem to happen since some commit, we can split until that commit.

What do you think about the new approach to the #clamp? I basically ported this: https://github.com/ruby/ruby/blob/master/compar.c#L222-L252 (but to Javascript this time) and I found out that the entire module can be simplified.

@hmdne
Copy link
Member Author

hmdne commented Jan 3, 2021

By the way, this force-push is only changed from f1c0c4f onwards (not counting the squashes).

@hmdne hmdne added this to the v1.1 milestone Jan 20, 2021
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

Great work!
(I refined a bit the commits so that each fixed spec is enabled along with the fix)

@elia elia merged commit e65a87d into opal:master Jan 21, 2021
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.

None yet

2 participants