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 #88

Merged
merged 11 commits into from Jun 12, 2018

Conversation

Projects
None yet
5 participants
@jasonrudolph
Member

jasonrudolph commented May 30, 2018

TODO

  • Decaffeinate lib/reporter.coffee
  • Decaffeinate lib/metrics.coffee
  • Replace CoffeeScript linting config with standard linting config

Verification Process

See #88 (comment) and #88 (comment).

/fyi @annthurium

jasonrudolph added some commits May 30, 2018

@jasonrudolph jasonrudolph changed the title from [WIP] ☠☕ Decaffeinate to ☠☕ Decaffeinate May 30, 2018

"coffeelint": "^1.9.7"
"standard": "*"
},
"standard": {

This comment has been minimized.

@50Wliu

50Wliu May 30, 2018

Member

To match other Atom packages that use standard, this should use the following block instead:

  "standard": {
    "env": [
      "browser",
      "node",
      "atomtest",
      "jasmine"
    ],
    "globals": [
      "atom"
    ]
  },

See for example https://github.com/atom/open-on-github/blob/58899b80b73f26dbe75e9938a61dfd3b8690de9e/package.json#L32-L42

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 12, 2018

Member

Thanks, @50Wliu. Updated in 023e95a.

@annthurium

This comment has been minimized.

Contributor

annthurium commented May 31, 2018

@jasonrudolph first of all, thank you so much for taking this on!

re smoke testing: is there a way to record network calls made from Atom? If so, you could compare the calls to Google Analytics when performing the same set of actions, before and after these changes

@thomasjo

This comment has been minimized.

Member

thomasjo commented May 31, 2018

re smoke testing: is there a way to record network calls made from Atom? If so, you could compare the calls to Google Analytics when performing the same set of actions, before and after these changes

@annthurium That's a really great idea! It's very easy to do in a (semi-)manual manner using e.g Wireshark.

@daviwil daviwil referenced this pull request Jun 11, 2018

Closed

Iteration Plan: May 29, 2018 #17426

8 of 17 tasks complete
@annthurium

This comment has been minimized.

Contributor

annthurium commented Jun 12, 2018

okay, so I did some manual smoke testing. I did this by looking at the network tab within the Chrome console in Atom, because Wireshark is kind of a pain when every app is constantly phoning home. :-)

tested the following actions on both the coffeescript and decaffeinated versions:

  • opening a file (with command t) sends a "file open" event
  • opening a file (by opening a new pane) sends a "file open" event
  • opting out of metrics sends an opt-out event
  • opting in to metrics sends an opt-in event
  • refreshing Atom windows sends a "window ended" event.
  • refreshing Atom window sends a "window started" event.
  • resfreshing Atom window sends timing/latency data.
  • deprecation. grim reports on packages that use old (< 1.0) atom apis. It took awhile to find a package that actually triggers a grim deprecation warning. I eventually looked in Google Analytics at real time deprecation warnings and was able to find one (atom-terminal-panel@4.4.4) and verify that the deprecation event is sent correctly.

possible issues, or things I wasn't confident I tested properly

  • I don't understand the watchCommands code. It looks like we're filtering out all commands that have a colon. Allll the commands in the command palette have colons. So what is this actually doing?
@thomasjo

This comment has been minimized.

Member

thomasjo commented Jun 12, 2018

I don't understand the watchCommands code. It looks like we're filtering out all commands that have a colon. Allll the commands in the command palette have colons. So what is this actually doing?

@annthurium I don't blame you; the original code was somewhat obtuse; it returns unless the result of indexOf(':') is greater than -1, which means it returns unless the event name contains a colon. In other words it filters out all events that are not commands. The expression would have been much easier to read if we had used a normal if as opposed to an unless. The new code is even less readable, and really should be rewritten to something like

if (eventName.indexOf(':') === -1) return

Hope my explanation was helpful.

jasonrudolph added some commits Jun 12, 2018

@@ -120,7 +120,7 @@ module.exports = {
if (eventName.startsWith('core:') || eventName.startsWith('editor:')) return
if (!(eventName.indexOf(':') > -1)) return
if (!eventName.includes(':')) return

This comment has been minimized.

@thomasjo

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 12, 2018

Member

@thomasjo: Thanks for pointing out this opportunity for improvement! ⚡️

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Jun 12, 2018

@annthurium: Thanks for taking the time to test these changes. 🙇🌟

possible issues, or things I wasn't confident I tested properly

  • I don't understand the watchCommands code. It looks like we're filtering out all commands that have a colon. Allll the commands in the command palette have colons. So what is this actually doing?

After reading @thomasjo's helpful explanation in #88 (comment), I refactored the code to make the intent more clear (hopefully 🤞 ). I then used the network tab in the dev tools to confirm that watch command events are recorded. 😅

@jasonrudolph jasonrudolph merged commit 18dbcb0 into master Jun 12, 2018

2 checks passed

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

@jasonrudolph jasonrudolph deleted the decaffeinate branch Jun 12, 2018

@annthurium

This comment has been minimized.

Contributor

annthurium commented Jun 12, 2018

thank you @thomasjo @jasonrudolph !!

@damieng

This comment has been minimized.

Contributor

damieng commented Jun 12, 2018

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