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

Convert Package to JS #16199

Merged
merged 2 commits into from Nov 29, 2017

Conversation

Projects
None yet
3 participants
@maxbrunsfeld
Contributor

maxbrunsfeld commented Nov 15, 2017

No description provided.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 16, 2017

Member

@maxbrunsfeld any chance this can wait until #16100 is merged?

Member

50Wliu commented Nov 16, 2017

@maxbrunsfeld any chance this can wait until #16100 is merged?

Show outdated Hide outdated src/package.js Outdated
Show outdated Hide outdated src/package.js Outdated
} catch (error) {
let version
try {
({version} = require(`${nativeModulePath}/package.json`))

This comment has been minimized.

@YurySolovyov

YurySolovyov Nov 16, 2017

are parens doing anything here?

@YurySolovyov

YurySolovyov Nov 16, 2017

are parens doing anything here?

This comment has been minimized.

@50Wliu

50Wliu Nov 16, 2017

Member

Yes, you cannot have a bracket start a line because it interprets it as a block.

@50Wliu

50Wliu Nov 16, 2017

Member

Yes, you cannot have a bracket start a line because it interprets it as a block.

}
measure (key, fn) {
const startTime = Date.now()

This comment has been minimized.

@YurySolovyov

YurySolovyov Nov 16, 2017

performance.now() could be less expensive and more precise here

@YurySolovyov

YurySolovyov Nov 16, 2017

performance.now() could be less expensive and more precise here

this.keymaps = []
for (const keymapPath in this.packageManager.packagesCache[this.name].keymaps) {
const keymapObject = this.packageManager.packagesCache[this.name].keymaps[keymapPath]
this.keymaps.push([`core:${keymapPath}`, keymapObject])

This comment has been minimized.

@YurySolovyov

YurySolovyov Nov 16, 2017

empty array + unconditional push seems like .map to me

@YurySolovyov

YurySolovyov Nov 16, 2017

empty array + unconditional push seems like .map to me

This comment has been minimized.

@maxbrunsfeld

maxbrunsfeld Nov 17, 2017

Contributor

This is a loop over an object, so for in is convenient. An alternative would be Object.keys(...).map, though that would allocate an intermediate array of keys. Readability-wise, they seem pretty much equivalent.

@maxbrunsfeld

maxbrunsfeld Nov 17, 2017

Contributor

This is a loop over an object, so for in is convenient. An alternative would be Object.keys(...).map, though that would allocate an intermediate array of keys. Readability-wise, they seem pretty much equivalent.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Nov 16, 2017

Contributor

any chance this can wait until #16100 is merged?

Absolutely. This is low priority. Just something easy to do when I need a break from working on another problem.

Contributor

maxbrunsfeld commented Nov 16, 2017

any chance this can wait until #16100 is merged?

Absolutely. This is low priority. Just something easy to do when I need a break from working on another problem.

@maxbrunsfeld maxbrunsfeld merged commit 3053069 into master Nov 29, 2017

1 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-decaffeinate-package branch Nov 29, 2017

@Arcanemagus Arcanemagus referenced this pull request Feb 14, 2018

Closed

atom.packages.loadPackage name resolution #16703

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