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 package source files #558

Merged
merged 10 commits into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@rafeca
Copy link
Contributor

commented Apr 23, 2019

Steps followed to decaffeinate the files:

  1. Use https://decaffeinate-project.org to convert coffeescript to JS.
  2. Address all suggestions manually.
  3. Run prettier-standard to make the code follow our JS linting rules.
Show resolved Hide resolved package.json Outdated

@rafeca rafeca force-pushed the decaffeinate branch from c783db2 to 8dbf7b0 Apr 23, 2019

@50Wliu
Copy link
Member

left a comment

Mostly just pointing out all the extra returns that were added, with a few other style comments

lib/main.js Outdated
},
'markdown-preview:toggle-break-on-single-newline': () => {
const keyPath = 'markdown-preview.breakOnSingleNewline'
return atom.config.set(keyPath, !atom.config.get(keyPath))

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 unnecessary return

lib/main.js Outdated
},
'markdown-preview:toggle-github-style': () => {
const keyPath = 'markdown-preview.useGitHubStyle'
return atom.config.set(keyPath, !atom.config.get(keyPath))

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 unnecessary return

lib/main.js Outdated
)
}
})
) // Do not return the results of the for loop

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 comment

lib/main.js Outdated
)
}

return this.disposables.add(

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 return

lib/main.js Outdated
.open(uri, options)
.then(function (markdownPreviewView) {
if (isMarkdownPreviewView(markdownPreviewView)) {
return previousActivePane.activate()

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 return

selectedNode != null &&
(this.element === selectedNode || this.element.contains(selectedNode))
) {
return atom.clipboard.write(selectedText)

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 return

) {
return atom.clipboard.write(selectedText)
} else {
return this.getHTML(function (error, html) {

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 return

This comment has been minimized.

Copy link
@rafeca

rafeca Apr 23, 2019

Author Contributor

This one is needed, since getHTML returns a Promise that needs to be awaited (I spent too much time debugging a failing test caused by this one 😅).

{ dismissable: true, detail: error.message }
)
} else {
return atom.clipboard.write(html)

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 return

} else {
return this.getHTML(function (error, html) {
if (error != null) {
return atom.notifications.addError(

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

🔥 return

})
})

describe("when the editor's path does not exist", () =>

This comment has been minimized.

Copy link
@50Wliu

50Wliu Apr 23, 2019

Member

For consistency, should we change all these into () => { } blocks, including the ones that are currently function () { }?

This comment has been minimized.

Copy link
@rafeca

rafeca Apr 23, 2019

Author Contributor

I can do that as well, what I've decided not to do is to change all the tests to use async/await and Promises: this would be too much work for now

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Thanks for pointing out the unneeded returns, I initially removed most of them but when I spent almost an hour debugging a test failure due to a couple of Promises that were not returned and therefore not awaited I decided to leave a lot of returns hehe

I've now removed all the ones that are unneeded.

@rafeca rafeca merged commit 16b4be5 into master Apr 23, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the decaffeinate branch Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.