Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Decaf /lib - #287

Merged
rafeca merged 9 commits into
atom:masterfrom
Aerijo:master
Apr 27, 2019
Merged

Decaf /lib#287
rafeca merged 9 commits into
atom:masterfrom
Aerijo:master

Conversation

@Aerijo

Copy link
Copy Markdown
Contributor

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Decaffeinates the CoffeeScript files inside the /lib directory.

Also fixes a spec that expected 1 console warning, but mine was giving 5 (it didn't say what they were). As the spec tests the message wording however, it should be safe to change toBe 1 to toBeGreaterThan 0

Alternate Designs

NA

Benefits

I can understand them now & work on PRs

Possible Drawbacks

FIrst time decaffeinating. May have missed some safe manual conversions. I did note a lot of foo != null, so I wasn't sure if values would be coming in as null or undefined and left them mostly alone.

Similar with return values (I hate implicit return...). All the specs passed, but that's as much as I can really guarantee. Presumably the return value of a method would be tested if it's important or can vary.

Applicable Issues

@Arcanemagus

Copy link
Copy Markdown

Looks like the AppVeyor container for the beta build had networking issues, I restarted the build for that.

@ArcanemagusArcanemagus left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Almost all of this is just changing for (let ...) to for (const ...) loops. decaffeinate emits for (let ...) since that's the safer option, and expects ESLint to be used to catch (and fix) cases where it can be written as for (const ...). Unfortunately the ESLint configuration used within standard is too simple to catch these automatically.

Other than that I saw a few minor style issues and a few things that actually require changes.

Comment threadlib/snippet-expansion.js Outdated
Comment threadlib/snippet-expansion.js Outdated
Comment threadlib/snippet-expansion.js Outdated
Comment threadlib/snippet-expansion.js Outdated
Comment threadlib/snippet.js Outdated
Comment threadlib/snippets.js Outdated
Comment threadlib/snippets.js Outdated
Comment threadlib/snippets.js Outdated
Comment threadlib/snippets.js Outdated
Comment threadlib/snippets.js Outdated
@Aerijo

Copy link
Copy Markdown
ContributorAuthor

@Arcanemagus I hope you didn't write out every comment each time... I'd have understood even with no comment 😄

@Arcanemagus

Copy link
Copy Markdown

Copy/paste meant it wasn't much more than the Ctrl + F to find them 😆.

@ArcanemagusArcanemagus left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM at least, now we just need somebody to give final approval.

@AerijoAerijo mentioned this pull request Feb 7, 2019
15 tasks

@rafecarafeca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks a lot!

I've glimpsed through the changes and didn't spot anything wrong, so we're good to go 😃

Comment threadlib/snippets.js Outdated
@rafecarafeca self-assigned this Apr 26, 2019
Co-Authored-By: Aerijo <bengray178@gmail.com>
@rafeca
rafeca merged commit bca7387 into atom:masterApr 27, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Aerijo@Arcanemagus@rafeca@rsese@asheren