Move to ES6 #503

lgeiger · 2016-11-15T02:21:54Z

Fixes #494

This will port all coffeescript code to ES6. There will be probably a few bugs since I didn't test all functionality.

n-riesco · 2016-11-15T07:15:59Z

lib/autocomplete-provider.js

+            let grammar = editor.getGrammar();
+            let language = kernelManager.getLanguageFor(grammar);
+            let kernel = kernelManager.getRunningKernelFor(language);
+            if (kernel == null) {


if(!kernel)

n-riesco · 2016-11-15T07:19:27Z

lib/code-manager.js

+
+        let foldable = this.editor.isFoldableAtBufferRow(row);
+        let foldRange = this.editor.languageMode.rowRangeForCodeFoldAtBufferRow(row);
+        if ((foldRange == null) || (foldRange[0] == null) || (foldRange[1] == null)) {


I guess this is how decaffeinate deals with ?. 😞

This should do the job:

if (!foldRange || !foldRange[0] || !foldRange[1])

n-riesco · 2016-11-15T07:24:02Z

lib/code-manager.js

-        if code?
-            return code.replace /\r\n|\r/g, '\n'
+    normalizeString(code) {
+        if (code != null) {


n-riesco · 2016-11-15T07:25:53Z

lib/code-manager.js

-        return range
+    getFoldRange(row) {
+        let range = this.editor.languageMode.rowRangeForCodeFoldAtBufferRow(row);
+        if (__guard__(this.getRow(range[1] + 1), x => x.trim()) === 'end') {


Oh! So decaffeinate sometimes uses a __guard__...

n-riesco · 2016-11-15T07:28:37Z

lib/code-manager.js


-        unless regexString?
-            return [start, end]
+        if (regexString == null) {


ditto (I'm going to stop marking these bugs)

I'll cleanup the rest. 👍

n-riesco · 2016-11-15T07:30:16Z

lib/code-manager.js

+    range.push(i);
+  }
+  return range;
+}


missing EOL

n-riesco · 2016-11-15T07:34:42Z

lib/code-manager.js

+function __guard__(value, transform) {
+  return (typeof value !== 'undefined' && value !== null) ? transform(value) : undefined;
+}
+function __range__(left, right, inclusive) {


I removed them in this commit.

n-riesco · 2016-11-15T07:49:20Z

lib/config.js

@@ -1,95 +1,54 @@
-_ = require 'lodash'
+let Config;


I reckon this line can go.

n-riesco · 2016-11-15T07:58:35Z

lib/kernel-manager.js

-        @_kernelSpecs = @getKernelSpecsFromSettings()
+export default class KernelManager {
+    constructor() {
+        this.getAllKernelSpecs = this.getAllKernelSpecs.bind(this);


Why is this necessary?

Hmm I actually have no idea.

Thanks, you're right. This can be removed 👍

n-riesco · 2016-11-15T07:58:45Z

lib/kernel-manager.js

+export default class KernelManager {
+    constructor() {
+        this.getAllKernelSpecs = this.getAllKernelSpecs.bind(this);
+        this.getKernelSpecsFromJupyter = this.getKernelSpecsFromJupyter.bind(this);


n-riesco · 2016-11-15T08:09:53Z

lib/main.js

-        marker = @editor.markBufferPosition
-            row: row
+// coffeelint: disable = missing_fat_arrows
+let Hydrogen;


n-riesco · 2016-11-15T08:11:44Z

lib/main.js

+        this.subscriptions = new CompositeDisposable;
+
+        this.subscriptions.add(atom.commands.add('atom-text-editor', {
+            ['hydrogen:run']: () => this.run(),


are the square brackets really necessary?

They are just an artifact from decaffeinate.

n-riesco · 2016-11-15T08:28:55Z

lib/plugin-api/hydrogen-kernel.js

+export default class HydrogenKernel {
+    constructor(_kernel) {
+        this._kernel = _kernel;
+        this.destroyed = false;


destroyed should belong to _kernel, since it's _kernel's responsibility to set it to true.

HydrogenKernel is just an API wrapper around Kernel. All the Kernel state should be kept in Kernel.

Note that destroyed is intentionally a part of the plugin API (i.e. plugins are expected and encouraged to use it). It can be switched to a property that proxies to _kernel, but removing it entirely is a breaking change. For a plugin API, stability is more important than perfection -- i.e. I would encourage not getting into the habit of making breaking changes.

n-riesco · 2016-11-15T08:31:23Z

lib/plugin-api/hydrogen-provider.js


-        return @_hydrogen.kernel.getPluginWrapper()
+        return this._hydrogen.kernel.getPluginWrapper();


Is building a wrapper so expensive that a getter makes a difference?

This is so that a plugin always gets the same wrapper object for the same kernel (same reference, not just equal-by-value). This should be true no matter the source API call, e.g. onDidChangeKernel vs. getActiveKernel.

Plugins need to be able to store metadata per-kernel. I decided to go with the approach of letting them attach custom fields to the wrapper object, and expecting the fields to be there later on. (It's possible to make a breaking change to some uuid-based thing, but that's separate from ES6 migration. Especially since hydrogen has no native notion of kernel ids.)

This is so that a plugin always gets the same wrapper object for the same kernel (same reference, not just equal-by-value).

Wouldn't you get the same wrapper if the wrapper is created by Kernel's constructor?

Plugins need to be able to store metadata per-kernel.

That's a bad pattern, because API users don't have any guarantee to avoid name clashes.
Composition would be simple and safer pattern:

let myH2Kernel = { kernel: kernelWrapper, myMetadata1: myMetadata1, myMetadata2: myMetadata2, };

Name clashes are definitely a real issue, but the pattern you suggest is not an ideal solution either. If you listen to onDidChangeKernel events, you get wrapper objects and not any custom struct. So each plug in will need to end up rolling their own dictionary lookup to fix that. I think adding some methods to encourage better namespacing (e.g. getData (namespace)), or figuring out how to return a different set of wrapper objects to each consumer, are more promising approaches.

Oh! I see where you're coming from. Equality should work in this case; e.g.:

> var a = {}; (function(paramA) { return paramA === a; })(a); true

n-riesco · 2016-11-15T08:36:11Z

lib/rename-view.js

+        this.cancel = this.cancel.bind(this);
+    }
+
+    static content(prompt) {


It doesn't make sense that this method is static. Static methods don't use this.

I refactored it in #508

n-riesco · 2016-11-15T08:43:10Z

lib/result-view.js

+                if (this._executionCount === null) {
+                    return 'Copy to clipboard';
+                } else {
+                    return `Copy to clipboard (Out[${this._executionCount}])`;


race condition? Is there any guarantee that title is invoked before the next _executionCount update? This is important for kernels that can update their output asynchronously.

Sorry, could you explain the race condition? I don't see any. Note that title() gets called during the mouseover event, not at construction-time.

My misunderstanding! I missed that addResult is only invoked with results for a given request (and thus a unique execution count).

n-riesco · 2016-11-15T08:47:01Z

lib/rename-view.js

-    }
+  constructor(...args) {
+    super(...args);
+    this.confirm = this.confirm.bind(this);


why is this necessary?

n-riesco · 2016-11-15T08:47:10Z

lib/rename-view.js

+  constructor(...args) {
+    super(...args);
+    this.confirm = this.confirm.bind(this);
+    this.cancel = this.cancel.bind(this);


n-riesco · 2016-11-15T08:49:34Z

lib/input-view.js

-    }
+  constructor(...args) {
+    super(...args);
+    this.confirm = this.confirm.bind(this);


why is this necessary?

n-riesco · 2016-11-15T08:49:47Z

lib/input-view.js

+  constructor(...args) {
+    super(...args);
+    this.confirm = this.confirm.bind(this);
+    this.close = this.close.bind(this);


n-riesco · 2016-11-15T08:51:28Z

lib/watch-sidebar.js

-        this.hide();
-        atom.workspace.addRightPanel({item: this.element});
+  constructor(kernel) {
+    this.resizeStarted = this.resizeStarted.bind(this);


why is this necessary?

n-riesco · 2016-11-15T08:51:41Z

lib/watch-sidebar.js

-        atom.workspace.addRightPanel({item: this.element});
+  constructor(kernel) {
+    this.resizeStarted = this.resizeStarted.bind(this);
+    this.resizeStopped = this.resizeStopped.bind(this);


n-riesco · 2016-11-15T08:51:51Z

lib/watch-sidebar.js

+  constructor(kernel) {
+    this.resizeStarted = this.resizeStarted.bind(this);
+    this.resizeStopped = this.resizeStopped.bind(this);
+    this.resizeSidebar = this.resizeSidebar.bind(this);


They are actually necessary. I think because they are used here.

n-riesco · 2016-11-15T09:07:24Z

I haven't been able to go through all the commits. Here are a few more comments:

Use const where appropriate.
Please, don't use 2-space indentation. Besides the readability issue, it doesn't discourage sloppy coding (with high cyclomatic complexity).
special care is needed with the translation of ?, because it's incorrect at many places.

n-riesco · 2016-11-15T09:13:11Z

.eslintrc.yml

+  no-underscore-dangle: [0]
+  no-useless-escape: [0]
+  prefer-rest-params: [0]
+  class-methods-use-this: [0]


I'm going to remove this again and use static methods as you mentioned above.

I'm going to remove this again and use static methods as you mentioned above.

My bad. This won't work.

I'm using this rule mainly because it would error in files like [kernel.js] since a lot of the methods don't use this.

n-riesco · 2016-11-15T09:14:21Z

.eslintrc.yml

+
+rules:
+  no-console: [0]
+  no-use-before-define: [0]


n-riesco · 2016-11-15T09:14:50Z

.eslintrc.yml

+  no-console: [0]
+  no-use-before-define: [0]
+  no-param-reassign: [0]
+  no-underscore-dangle: [0]


are we using underscore dangle?

Yep we have methods like _execute etc.

n-riesco · 2016-11-15T09:16:46Z

.eslintrc.yml

+  no-use-before-define: [0]
+  no-param-reassign: [0]
+  no-underscore-dangle: [0]
+  no-useless-escape: [0]


Otherwise it would complain about this line

That's actually a bug, isn't it?

Shouldn't it be?

const regexString = `${escapedCommentStartString}(%%| %%| <codecell>| In\\[[0-9 ]*\\]:?)`;

This way we keep the git history.

and add babel-eslint to dependencies

Caused by if(0) returning false

lgeiger · 2016-11-21T11:37:43Z

Rebased

lgeiger added the needs testing 🚧 label Nov 15, 2016

lgeiger force-pushed the js branch from 057c018 to 4295cb1 Compare November 15, 2016 02:25

n-riesco reviewed Nov 15, 2016

View reviewed changes

lib/code-manager.js

range.push(i);

}

return range;

}

Copy link

Collaborator

n-riesco Nov 15, 2016

Choose a reason for hiding this comment

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

missing EOL

lgeiger reacted with thumbs up emoji

n-riesco reviewed Nov 15, 2016

View reviewed changes

lib/rename-view.js

}

constructor(...args) {

super(...args);

this.confirm = this.confirm.bind(this);

Copy link

Collaborator

n-riesco Nov 15, 2016

Choose a reason for hiding this comment

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

why is this necessary?

n-riesco reviewed Nov 15, 2016

View reviewed changes

lib/input-view.js

}

constructor(...args) {

super(...args);

this.confirm = this.confirm.bind(this);

Copy link

Collaborator

n-riesco Nov 15, 2016

Choose a reason for hiding this comment

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

why is this necessary?

n-riesco reviewed Nov 15, 2016

View reviewed changes

lgeiger removed the needs testing 🚧 label Nov 17, 2016

lgeiger mentioned this pull request Nov 18, 2016

Refactor input views #508

Closed

lgeiger added 22 commits November 21, 2016 12:35

Rename files to prepare for conversion to JavaScript.

23000a1

This way we keep the git history.

Decaffeinate

7817040

Beautify *.js to fix indentation

b35f33f

ESLINT

16c37c5

Move guards to module

9ec5b8a

Use babel

46d041b

Fix bugs introduced by fixing lint errrors

3da959d

Convert tests to JS

f2447ac

Remove unnecessary returns and __guardFunc__

e89256d

Fix API docs script

012aba1

and add babel-eslint to dependencies

Replace guards with plain JS

194d9ee

Fix toggle watch sidebar

14ac119

Fix null comparisons

82231fd

Cleanup variable initialisation.

0fabe8d

Remove noop functions

3ae0a2f

Fix error when running code in line 0

0e2edd4

Caused by if(0) returning false

Fix switch-kernel command

a86c1f7

Fix exeption caused by null.trim()

c9f1022

Fix jupyter-js-services import

35a8186

Fix remote kernel restart

3f63e6f

Fix input views

ace1d18

Remove unnecessary bound functions

d33051e

lgeiger force-pushed the js branch from e2718af to d33051e Compare November 21, 2016 11:37

lgeiger mentioned this pull request Nov 21, 2016

Refactor input view #513

Merged

rgbkrk merged commit ef6d826 into nteract:master Nov 21, 2016

lgeiger deleted the js branch November 21, 2016 18:40

n-riesco mentioned this pull request Jul 31, 2018

Don't trim code selection before sending to kernel #1363

Merged


		return @_hydrogen.kernel.getPluginWrapper()
		return this._hydrogen.kernel.getPluginWrapper();

Move to ES6 #503

Move to ES6 #503

Conversation

lgeiger commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgeiger Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco commented Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgeiger Nov 15, 2016 • edited by rgbkrk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgeiger commented Nov 21, 2016

lgeiger Nov 15, 2016 •

edited

Loading

n-riesco Nov 15, 2016 •

edited

Loading

n-riesco Nov 15, 2016 •

edited

Loading

n-riesco commented Nov 15, 2016 •

edited

Loading

lgeiger Nov 15, 2016 •

edited by rgbkrk

Loading