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 `src/project.coffee` #15898

Merged
merged 14 commits into from Oct 17, 2017

Conversation

Projects
None yet
3 participants
@jasonrudolph
Member

jasonrudolph commented Oct 15, 2017

This change converts the Project class to JavaScript with standard style.

jasonrudolph added some commits Oct 14, 2017

☠️☕ Decaffeinate src/project.coffee
Apply results of running:

```
$ decaffeinate --keep-commonjs --prefer-const --loose-default-params --loose-for-expressions --loose-for-of --loose-includes'

$ standard --fix src/project.js
```
🐛 Fix bug disposing watchers in Project::addPath
`this.rootDirectories` is an Array of Directory objects. `path` is a
String. Therefore, `this.rootDirectories.includes(path)` will always
evaluate to `false`. We instead need to look for an entry in
`this.rootDirectories` where the Directory object's path is equal to the
given path.
@jasonrudolph

@smashwilson: This decaffeination effort involved some manual tweaking to the watcher-related code in the Project class. (See comments below.) Given your recent work on the watcher bits, would you mind offering a sanity check of the changes described in the comments below? I think the changes are reasonable, but I'd feel more confident about those changes if they had another set of 👀. 😄

for (let path in this.watcherPromisesByPath) {
this.watcherPromisesByPath[path].then(watcher => { watcher.dispose() })
}

This comment has been minimized.

@jasonrudolph

jasonrudolph Oct 15, 2017

Member

The CoffeeScript version of this code looked like this:

watcher.then((w) -> w.dispose()) for _, watcher in @watcherPromisesByPath

decaffeinate translated that to the following JavaScript:

for (let watcher = 0; watcher < this.watcherPromisesByPath.length; watcher++) { _ = this.watcherPromisesByPath[watcher]; watcher.then(w => w.dispose()) }

That seems like a non-equivalent translation. watcherPromisesByPath is initialized as an object literal, so watcherPromisesByPath.length is undefined.

To address this issue, 5937b95 replaces decaffeinate's translation with the for loop shown above.

@jasonrudolph

jasonrudolph Oct 15, 2017

Member

The CoffeeScript version of this code looked like this:

watcher.then((w) -> w.dispose()) for _, watcher in @watcherPromisesByPath

decaffeinate translated that to the following JavaScript:

for (let watcher = 0; watcher < this.watcherPromisesByPath.length; watcher++) { _ = this.watcherPromisesByPath[watcher]; watcher.then(w => w.dispose()) }

That seems like a non-equivalent translation. watcherPromisesByPath is initialized as an object literal, so watcherPromisesByPath.length is undefined.

To address this issue, 5937b95 replaces decaffeinate's translation with the for loop shown above.

This comment has been minimized.

@smashwilson

smashwilson Oct 16, 2017

Member

👍 Your translation is spot-on to the intention of the original. It looks like I used an in comprehension when I should have used an of loop in the original!

@smashwilson

smashwilson Oct 16, 2017

Member

👍 Your translation is spot-on to the intention of the original. It looks like I used an in comprehension when I should have used an of loop in the original!

Show outdated Hide outdated src/project.js Outdated
@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Oct 16, 2017

Contributor

☕️

Contributor

leroix commented Oct 16, 2017

☕️

@jasonrudolph jasonrudolph merged commit 274b809 into master Oct 17, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasonrudolph jasonrudolph deleted the decaf-project branch Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment