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 clean spec #66

Merged
merged 4 commits into from
May 21, 2023
Merged

Conversation

2colours
Copy link
Contributor

@2colours 2colours commented Apr 8, 2023

No description provided.

@2colours 2colours mentioned this pull request Apr 8, 2023
12 tasks
@2colours 2colours marked this pull request as ready for review April 8, 2023 15:23
@2colours
Copy link
Contributor Author

2colours commented Apr 8, 2023

This is meant to supersede #22.
Like with #65, I was trying to stay away from require-ing the config file while incorporating the configurable node version commits. Also, eliminated some closure variables with their initialization.

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.

The changes here look good, and the relevant tests are passing

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

👍

As @confused-Techie said, the tests pass and the changes look okay.

Some stuff I noticed, but not blocking the PR:

I don't totally understand the aversion to requiring a JSON file, but I suppose the change isn't worth worrying about either. EDIT: It would be too much hassle to start worrying about this in this PR when the approach was already accepted in #65, so disregard this comment, I guess.

There is a redundant commit deleting spec/spec-helper.coffee even though it is also deleted on the target decaf branch, this will add a little noise to the commit log but beyond that it is a non-issue, just something I saw and will point out as I'm reviewing. Again, I don't think this needs to change -- I would be fine merging this as-is. If in the future you rebase commits for any decaf PRs, you can leave out the commit deleting spec/spec-helper.coffee for cleanest/simplest commit history. But not a huge deal.

@2colours
Copy link
Contributor Author

I don't totally understand the aversion to requiring a JSON file, but I suppose the change isn't worth worrying about either. EDIT: It would be too much hassle to start worrying about this in this PR when the approach was already accepted in #65, so disregard this comment, I guess.

Anyway... I would hope that I said my biggest arguments under that PR - of course, these are still rather minor things but then the change itself is rather minor, too. It's a bit bothering that the code needs to rely on CommonJS either way. :(

There is a redundant commit deleting spec/spec-helper.coffee even though it is also deleted on the target decaf branch, this will add a little noise to the commit log but beyond that it is a non-issue, just something I saw and will point out as I'm reviewing. Again, I don't think this needs to change -- I would be fine merging this as-is. If in the future you rebase commits for any decaf PRs, you can leave out the commit deleting spec/spec-helper.coffee for cleanest/simplest commit history. But not a huge deal.

Yep... honestly, I never quite understood merge. I understand rebase and I thought the outcome would essentially be the same but apparently not, yuck. I just wanted to avoid PRs building on top of each other.
I don't know if you can rebase these branches but even at the end, I suppose an interactive rebase can help clear up unwanted commits, in case.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

Same approval reasons as before, plus the requested whitespace-only change was made. Thank you. 👍

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.

4 participants