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

Decaffeinate spec helper (updated) #63

Merged

Conversation

2colours
Copy link
Contributor

No description provided.

@2colours 2colours force-pushed the decaffeinate-spec-helper branch from d3c756c to dddbe31 Compare March 28, 2023 11:27
@2colours 2colours marked this pull request as ready for review March 28, 2023 11:28
@2colours
Copy link
Contributor Author

This is basically the updated version of #19, targeting the dedicated branch for decaffeinization. The only thing I changed on principle is the semicolons: I think we shouldn't go for the "automatic semicolon insertion" rules. Explicit is better than implicit with this surely.

If you think the coffee file shouldn't be removed, let me know and I will drop that commit.

@confused-Techie
Copy link
Member

@2colours This looks fantastic!

Totally agree, automatic semi-colon insertion I feel like is messy and harder to read, so a fan of them being put back in.

I'm seeing you also got ride of the map to invoke andCallThrough(), not totally needed, but does make things a little less messy, and looks like it'd still do the exact same thing, so super rad!

And sure thing, removing the CoffeeScript at this point seems like a fine move to me, and with all that said

  • spec/spec-helper.js: ✅

So if you wanna continue with this process by all means, and hope you can see what I mean by how much easier it is to review.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Now knowing the decaf PRs are being done separately I can review them individual. (Wasn't sure on the first review I left here).

But this PR looks good, all tests are passing, and the decaf work here isn't very extensive. If anything this PR is likely more a nice refactoring. But LGTM!

Thanks a ton for this contribution

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