UAST viewer #12

dpordomingo · 2017-06-30T07:30:45Z

issue: https://github.com/src-d/backlog/issues/816

The examples are fetched from files, and uast from static json built from a fixed version of the UAST, returned by bblfsh python-client.

Pending:

tests

Nodes having start_position and end_position

Nodes having only start_position (no end_position)

erizocosmico

Also, tests are not passing

erizocosmico · 2017-06-30T08:14:47Z

.env.local

@@ -0,0 +1 @@
+REACT_EDITOR=code


that should not be committed

erizocosmico · 2017-06-30T08:15:32Z

src/App.js

@@ -8,6 +8,22 @@ import Editor from './components/Editor';
 import UASTViewer from './components/UASTViewer';
 import languages from './languages';

+import {code_py} from './examples/example.py.js';


spaces after { and before } in imports.

No snake case for variables

erizocosmico · 2017-06-30T08:16:31Z

src/App.js

@@ -60,6 +80,11 @@ export default class App extends Component {
    this.setState({ selectedLanguage });
  }

+  onExampleChanged(e) {
+    const selectedExample = e.target.value;


why add another variable declaration when selectedExample is almost as long as e.target.value?

erizocosmico · 2017-06-30T08:18:26Z

src/components/UASTViewer.js

@@ -1,18 +1,35 @@
 import React, { Component } from 'react';
 import styled from 'styled-components';
+import Node from './uast/Node';
+import {eventHandler} from './uast/EventHandler';


erizocosmico · 2017-06-30T08:20:19Z

src/examples/example.java

@@ -0,0 +1,7 @@
+public class HelloWorld


either we have it in js or as a file, why both?

erizocosmico · 2017-06-30T08:20:51Z

src/examples/example.py

@@ -0,0 +1,2 @@
+import sys


erizocosmico · 2017-06-30T08:22:41Z

src/components/UASTViewer.js

 `

 export default class UASTViewer extends Component {
+  constructor(props) {
+    super(props);
+    eventHandler.onNodeSelected = this.props.onNodeSelected;


i'd move all the logic in eventhandler into the UASTViewer, it really makes no sense to have it outside when it's clearly the responsibility of this component.

If I'm understanding this well, EventHandler is just a singleton class that is used as a hack to not having to pass onNodeSelected down to all children, amirite? I'd rather have this done the right way.

Extracting non-core and non-main functionalities to external services/components/whatever helps to understand and test code; I can push everything in the very same file, but I think it does not help at all to improve the quality of the app

Having a singleton class that exists only to not pass around the onNodeSelected as prop does not help the quality (imho) or make the code easier to understand. And it does complicate testing a lot. Whereas passing the onNodeSelected as prop you can pass a jest.fn() to the component instead of having to set the callback on the singleton.

Also, selecting the nodes seems pretty core functionality of the component to me.

It is not a singleton but a class instantiated only once.
I prefer much more anything closer to throwing events, than passing callback deeper and deeper everywhere, but I can refactor this piece for sure

Whose default export is the instance. That is, a singleton.
But that's not how things are done the react way. Parents pass callbacks down as props, children call them.

erizocosmico · 2017-06-30T08:24:33Z

src/components/uast/EventHandler.js

@@ -0,0 +1,37 @@
+class EventHandler {
+  constructor() {
+    console.error("CREATED EventHandler")


erizocosmico · 2017-06-30T08:24:38Z

src/components/uast/EventHandler.js

+  }
+
+  changedCursor(position) {
+    console.log(position);


this is the missing part, where I'm currently working

erizocosmico · 2017-06-30T08:25:09Z

src/components/uast/EventHandler.js

+      };
+    }
+
+    return function (e){


space between ) and {

erizocosmico · 2017-06-30T08:30:02Z

Btw, you see the body with a serif font?

Oh, and the uast viewer (as far as I can see in the screenshot) seems really cool, awesome job! 👍

Serabe

Awesome job.

Serabe · 2017-06-30T09:00:08Z

src/components/Editor.js

-    return this.document.markText(from, to, {
-      css: 'background: yellow',
-    });
+  selectCode(from, to) {debugger;


s/debugger//

Also, debugger is a leftover

Serabe · 2017-06-30T09:00:48Z

src/components/Editor.js

+        css: 'background: yellow',
+      });
+    } else if (from) {
+      return this.document.setBookmark(from, {widget: this.bookmark});


Space after { and before }

Serabe · 2017-06-30T09:07:17Z

src/components/uast/EventHandler.js

+  }
+
+  selectedNodeTrigger(node) {
+    const nodeSelectedCallback = this.nodeSelectedCallback;


const { nodeSelectedCallback } = this;

Serabe · 2017-06-30T09:08:32Z

src/components/uast/EventHandler.js

+      to = {
+        line: node.EndPosition.Line - 1,
+        ch: node.EndPosition.Col - 1
+      };


Code repetition in from and to setting.

Serabe · 2017-06-30T09:11:44Z

src/components/uast/Node.js

+  return (
+    name ?
+      <StyledPropertyName>{name}</StyledPropertyName> :
+      null


If we are going to use multiline ternary operators, at least let's write them everytime the same way:

cond ? if_true : if_false

Serabe · 2017-06-30T09:20:04Z

src/components/uast/Node.js

+        )
+      }
+    });
+  }


Extract to function.

coordinatesForPosition(position) { if (!position) { return []; } const values = ['Offset', 'Line', 'Col']; return values .filter(x => typeof position[x] !== 'undefined') .map((x, i) => <Property key={i} name={name.toLowerCase()} value={position[name]} />); }

Serabe · 2017-06-30T09:20:18Z

src/components/uast/Node.js

+        { coordinates }
+      </CollapsibleItem>
+    );
+  } else {


No else in these cases. Just return.

Serabe · 2017-06-30T09:20:27Z

src/components/uast/Node.js

+  }
+}
+
+function Roles({roles}) {


Destructuring braces.

Serabe · 2017-06-30T09:23:06Z

src/components/uast/Node.js

+  }
+
+  render() {
+    const {name, label, children, onFocusNode} = this.props;


Destructuring spaces.

Serabe · 2017-06-30T09:24:16Z

src/index.js

@@ -5,6 +5,7 @@ import { injectGlobal } from 'styled-components';
 import App from './App';
 import registerServiceWorker from './registerServiceWorker';
 import './vendor.css';
+import {font} from './styling/variables';


Space after and before { }

dpordomingo · 2017-06-30T13:12:52Z

All your comments were considered in the last commit, that includes the listener for cursor changes

Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>

erizocosmico · 2017-06-30T13:40:20Z

Remember to fix the tests.

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>

dpordomingo requested review from Serabe and erizocosmico June 30, 2017 07:30

dpordomingo changed the title ~~Astviewer~~ UAST viewer Jun 30, 2017

erizocosmico suggested changes Jun 30, 2017

View reviewed changes

Serabe suggested changes Jun 30, 2017

View reviewed changes

dpordomingo force-pushed the astviewer branch from 45966a9 to d4f32f6 Compare June 30, 2017 13:35

UAST viewer panel

984e63e

Signed-off-by: David Pordomingo <David.Pordomingo.F@gmail.com>

dpordomingo force-pushed the astviewer branch from d4f32f6 to 984e63e Compare June 30, 2017 13:37

erizocosmico approved these changes Jun 30, 2017

View reviewed changes

erizocosmico added 3 commits June 30, 2017 15:57

fix tests

23be021

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>

add test checking onExampleChanged is called on dropdown change

ef99d9e

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>

keep cursor position on update code

88abd87

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>

Serabe approved these changes Jul 3, 2017

View reviewed changes

Serabe merged commit 69a8704 into bblfsh:master Jul 3, 2017

dpordomingo deleted the astviewer branch November 10, 2017 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UAST viewer #12

UAST viewer #12

dpordomingo commented Jun 30, 2017 •

edited

Loading

erizocosmico left a comment

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

dpordomingo Jun 30, 2017 •

edited

Loading

erizocosmico Jun 30, 2017

dpordomingo Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico Jun 30, 2017

dpordomingo Jun 30, 2017

erizocosmico Jun 30, 2017

erizocosmico commented Jun 30, 2017 •

edited

Loading

Serabe left a comment

Serabe Jun 30, 2017

erizocosmico Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

Serabe Jun 30, 2017

dpordomingo commented Jun 30, 2017

erizocosmico commented Jun 30, 2017

		@@ -0,0 +1 @@
		REACT_EDITOR=code

UAST viewer #12

UAST viewer #12

Conversation

dpordomingo commented Jun 30, 2017 • edited Loading

erizocosmico left a 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

dpordomingo Jun 30, 2017 • 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

erizocosmico commented Jun 30, 2017 • edited Loading

Serabe left a 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

dpordomingo commented Jun 30, 2017

erizocosmico commented Jun 30, 2017

dpordomingo commented Jun 30, 2017 •

edited

Loading

dpordomingo Jun 30, 2017 •

edited

Loading

erizocosmico commented Jun 30, 2017 •

edited

Loading