code review 170040043: go-tour: add error highlighting

go-tour: add error highlighting

[francesc, 2014-11-06 02:09:12]

Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go-tour

[francesc, 2014-11-06 02:18:54]

Change is live on campoy.cat/gotour

On Wed, Nov 5, 2014 at 6:09 PM, <campoy@golang.org> wrote:

> Reviewers: adg,
>
> Message:
> Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go-tour
>
>
> Description:
> go-tour: add error highlighting
>
> Please review this at https://codereview.appspot.com/170040043/
>
> Affected files (+48, -5 lines):
>   M static/css/app.css
>   M static/js/services.js
>   M static/js/values.js
>
>
> Index: static/css/app.css
> ===================================================================
> --- a/static/css/app.css
> +++ b/static/css/app.css
> @@ -259,6 +259,13 @@
>      font-family:'Inconsolata', monospace;
>      line-height: 1.2em;
>  }
> +.CodeMirror-code > .line-error {
> +    background: #FF8080;
> +}
> +.CodeMirror-code > .line-error .CodeMirror-linenumber {
> +    color: #FF5555;
> +    font-weight: bolder;
> +}
>  #file-editor .CodeMirror-gutters {
>      width: 32px;
>  }
> Index: static/js/services.js
> ===================================================================
> --- a/static/js/services.js
> +++ b/static/js/services.js
> @@ -31,14 +31,28 @@
>  ]).
>
>  // Running code
> -factory('run', ['$window',
> -    function(win) {
> +factory('run', ['$window', 'editor',
> +    function(win, editor) {
> +        var writeInterceptor = function(writer) {
> +            return function(write) {
> +                if (write.Kind == 'stderr') {
> +                    var lines = write.Body.split('\n');
> +                    for (var i in lines) {
> +                        var match = lines[i].match(/prog\.go:([0-9]+):
> ([^\n]*)/);
> +                        if (match !== null) {
> +                            editor.highlight(match[1], match[2]);
> +                        }
> +                    }
> +                }
> +                writer(write);
> +            };
> +        };
>          return function(code, output, options) {
>              // PlaygroundOutput is defined in playground.js which is
> prepended
>              // to the generated script.js in gotour/tour.go.
>              // The next line removes the jshint warning.
>              // global PlaygroundOutput
> -            win.transport.Run(code, PlaygroundOutput(output), options);
> +            win.transport.Run(code,
writeInterceptor(PlaygroundOutput(output)),
> options);
>          };
>      }
>  ]).
> @@ -85,6 +99,7 @@
>  // Editor context service, kept through the whole app.
>  factory('editor', ['$window', 'storage',
>      function(win, storage) {
> +        var highlightedErrors = [];
>          var ctx = {
>              syntax: storage.get('syntax') === 'true',
>              toggleSyntax: function() {
> @@ -108,7 +123,22 @@
>                  };
>                  set();
>              },
> +            highlight: function(line, message) {
> +                var l = $('.CodeMirror-code > div:nth-child(' + line +
> ')');
> +                l.addClass('line-error').attr('title', message);
> +                highlightedErrors.push(l);
> +            },
> +            onChange: function() {
> +                for (var i in highlightedErrors) {
> +                    highlightedErrors[i]
> +                        .removeClass('line-error').attr('title', null);
> +                }
> +                highlightedErrors = [];
> +            }
>          };
> +        // Set in the window so the onChange function in the codemirror
> config
> +        // can call it.
> +        win.codeChanged = ctx.onChange;
>          return ctx;
>      }
>  ]).
> @@ -171,7 +201,8 @@
>                  }
>                  moduleQ.resolve(modules);
>                  lessonQ.resolve(lessons);
> -            }, function(error) {
> +            },
> +            function(error) {
>                  $log.error('error loading lessons : ', error);
>                  moduleQ.reject(error);
>                  lessonQ.reject(error);
> Index: static/js/values.js
> ===================================================================
> --- a/static/js/values.js
> +++ b/static/js/values.js
> @@ -72,6 +72,11 @@
>              'PageUp': function() {
>                  return false;
>              },
> -        }
> +        },
> +        // TODO: is there a better way to do this?
> +        // AngularJS values can't depend on factories.
> +        onChange: function() {
> +            if (window.codeChanged !== null) window.codeChanged();
> +        },
>      }
>  });
>
>
>


-- 
--
Francesc Campoy
http://twitter.com/francesc

[adg1, 2014-11-06 04:31:32]

https://codereview.appspot.com/170040043/diff/60001/static/js/services.js
File static/js/services.js (right):

https://codereview.appspot.com/170040043/diff/60001/static/js/services.js#new...
static/js/services.js:132: for (var i in highlightedErrors) {
we don't need to keep track of the highlighted lines
just
  $('.line-error').removeClass('line-error').attr('title', null)
??

https://codereview.appspot.com/170040043/diff/60001/static/js/values.js
File static/js/values.js (right):

https://codereview.appspot.com/170040043/diff/60001/static/js/values.js#newco...
static/js/values.js:80: },
no trailing comma

[francesc, 2014-11-06 19:20:48]

https://codereview.appspot.com/170040043/diff/60001/static/js/services.js
File static/js/services.js (right):

https://codereview.appspot.com/170040043/diff/60001/static/js/services.js#new...
static/js/services.js:132: for (var i in highlightedErrors) {
On 2014/11/06 04:31:32, adg1 wrote:
> we don't need to keep track of the highlighted lines
> just
>   $('.line-error').removeClass('line-error').attr('title', null)
> ??

I did that thinking about performance ... probably prematurely.

https://codereview.appspot.com/170040043/diff/60001/static/js/values.js
File static/js/values.js (right):

https://codereview.appspot.com/170040043/diff/60001/static/js/values.js#newco...
static/js/values.js:80: },
On 2014/11/06 04:31:32, adg1 wrote:
> no trailing comma

Done.

[adg1, 2014-11-06 22:29:43]

LGTM

[francesc, 2014-11-11 19:07:02]

*** Submitted as https://code.google.com/p/go-tour/source/detail?r=da9e333fda8e
***

go-tour: add error highlighting

LGTM=adg
R=adg
CC=golang-codereviews
https://codereview.appspot.com/170040043