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

Decaf wrap-guide #443

Merged
merged 1 commit into from
Apr 9, 2023
Merged

Decaf wrap-guide #443

merged 1 commit into from
Apr 9, 2023

Conversation

confused-Techie
Copy link
Member

So this PR is a bit of a smaller one, some simple decaf work for the package wrap-guide.

As always this PR is targeting a branch wrap-guide-decaf that has been machine decaffed for easier review.

If this PR is accepted, prior to being merged, the target branch will change to main and will be merged into the editor.

Please feel free to provide any feedback!

A few notes here, some of the awkward return function patterns were left in to be reworked later in a more focused PR, since they do work, and the fix isn't immediately known. Additionally within wrap-guide-element.js on Line 32, there is a TODO once it's converted to JavaScript. I'll take a further look, since I'm unsure of which portion should be changed, but I'm assuming it's the API call itself to getNextUpdatePromise() but will try to investigate further later on.

@confused-Techie
Copy link
Member Author

So on this:

  • find-and-replace: 45 Failures ✅
  • settings-view: 1 Failure ❌ (But we are seeing this repo wide, and has no bearing on this PR)
  • symbols-view: 2 Failures ✅
  • tree-view: 2 Failures ✅

So tests look good, looks like this decaf went without any issues

@confused-Techie
Copy link
Member Author

If anyone wants to give this a review it'd be very appreciated

Copy link
Member

@Spiker985 Spiker985 left a comment

Choose a reason for hiding this comment

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

It all reads properly. The single } on L187 is correct, yeah? (reviewing on mobile - it's a pain to make sure brackets match across long swaths of code)

And I assume no spec changes (at this time), because otherwise how would we know if the decaff broke anything, right?

@confused-Techie
Copy link
Member Author

@Spiker985 Yup zero spec changes, and that single } is because the whole class was originally wrapped in () rather than being normally declared. So that last bracket is the end of a class definition. And all tests are still passing, thanks a ton for giving such a speedy review!

@confused-Techie
Copy link
Member Author

@Spiker985 thanks for your approval!
With that and passing specs, I'll go ahead and merge this one!

@confused-Techie confused-Techie merged commit 222a31d into wrap-guide-decaf Apr 9, 2023
@confused-Techie confused-Techie deleted the wrap-guide-decaf-ct branch April 9, 2023 20:53
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