legpad.py (python script that generates legpad.html files)

legpad.py (python script that generates legpad.html files)


BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=6041

[floitsch, 2012-03-28 05:48:49]

LGTM, but please wait for Peter's review.

https://chromiumcodereview.appspot.com/9866022/diff/2001/tools/testing/legpad...
File tools/testing/legpad/legpad.py (right):

https://chromiumcodereview.appspot.com/9866022/diff/2001/tools/testing/legpad...
tools/testing/legpad/legpad.py:124: # TODO - read this from some config file
once ahe/zundel create it
TODO(ldap):

[mattsh, 2012-03-28 15:29:10]

Thanks Florian.  Peter, over to you...

https://chromiumcodereview.appspot.com/9866022/diff/2001/tools/testing/legpad...
File tools/testing/legpad/legpad.py (right):

https://chromiumcodereview.appspot.com/9866022/diff/2001/tools/testing/legpad...
tools/testing/legpad/legpad.py:124: # TODO - read this from some config file
once ahe/zundel create it
On 2012/03/28 05:48:49, floitsch wrote:
> TODO(ldap):

Done.

[ahe, 2012-03-28 18:38:27]

Comments so far. I'll continue tomorrow.

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
File tools/testing/legpad/legpad.py (right):

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:6: #
Extra #

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:9: Legpad is used to compile .dart files to
javascript, using the leg (now
remove "leg (now"

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:10: called dart2js) compiler.
remove "called" and ")".

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:30: have the generated javascript.
I was having a hard time following these instructions. Perhaps you should
mention that you need to compile legpad.dart? A complete example would be nice.

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:226: drt += ".app"
Should be: ".app/Contents/MacOS/DumpRenderTree"

[mattsh, 2012-03-28 19:59:59]

OK, thanks for quick feedback.  I've added an example.dart file now, along with
a script showing how to generate the legpad.html for it.
Also, I've updated all the paths to reflect the move of the dart2js compiler.
(Now hitting an issue internal to the dart2js tokenizer, but that is independent
of this CL.)

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
File tools/testing/legpad/legpad.py (right):

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:6: #
On 2012/03/28 18:38:28, ahe wrote:
> Extra #

Done.

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:9: Legpad is used to compile .dart files to
javascript, using the leg (now
On 2012/03/28 18:38:28, ahe wrote:
> remove "leg (now"

Done.

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:10: called dart2js) compiler.
On 2012/03/28 18:38:28, ahe wrote:
> remove "called" and ")".

Done.

https://chromiumcodereview.appspot.com/9866022/diff/7001/tools/testing/legpad...
tools/testing/legpad/legpad.py:30: have the generated javascript.
On 2012/03/28 18:38:28, ahe wrote:
> I was having a hard time following these instructions. Perhaps you should
> mention that you need to compile legpad.dart? A complete example would be
nice.

Good suggestion.  I'm adding a simple bash script and an example.dart file.

[ahe, 2012-03-30 08:55:32]

Hi Matt,

Thank you so much for giving us this tool to help us debug self-hosting!

LGTM, if you plan to follow up with a CL to address the build issues, and source
directory hygiene issues.

The rest is just nits.

I would really prefer if this was done in Dart.

Cheers,
Peter

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
File tools/testing/legpad/example.dart (right):

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.dart:1: void main() {
No copyright.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
File tools/testing/legpad/example.sh (right):

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:1: # sample script how to use legpad
No copyright.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:1: # sample script how to use legpad
#!/bin/bash

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:3: # first compile "legpad.dart" to
"legpad.dart.js" usung dart2js
using

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:3: # first compile "legpad.dart" to
"legpad.dart.js" usung dart2js
Not a proper sentence. Start with a capitalized word and end with a period.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:4: set -x
set -x -e

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:8: # now run legpad to generate an html page
that can be used to compile example.dart
Long line, not a proper sentence.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:9: python legpad.py example.dart
exec python ...

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/exampl...
tools/testing/legpad/example.sh:10: 
Extra line.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
File tools/testing/legpad/legpad.dart (right):

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:24: // id of script element containing name of
the main dart file
Not a proper comment, and not a documentation comment.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:30: // accumulates diagnostic messages emitted
by the leg compiler
Ditto.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:33: // the generated javascript
Ditto.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:53: warnings += sb.toString();
I think this is a weird change. You clearly need a string buffer.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:78: output = "throw 'legpad compilation
error';\n";
legpad -> dart2js.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:95: // TODO(mattsh) - dart2js api should be
synchronous
The form is this:

TODO(ldap): Blah blah blah.

A proper sentence and a colon, not a hyphen.

Also, you're not going to get anywhere statements of opinion about an API in
code that is using that API. Instead, you'd have to add the TODO to the API
(which you shouldn't or file a bug).

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:100: output = "throw 'legpad compilation
error';\n";
legpad -> dart2js

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.dart:122: // TODO(mattsh): should exist in standard
lib somewhere
Not a proper sentence.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad.py
File tools/testing/legpad/legpad.py (right):

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:1: #!/usr/bin/env python
I'm saddened by how much code is in this CL that is not written in Dart. As a
team, I think we need to change this practice.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:11: <something>.legpad.html) that executes the
dart2js compiler when the page
legpad.py requires a file in this directory named legpad.dart.js. This pollutes
the source directory, and so does the HTML file legpad creates. I don't
understand why this is not built with GYP.

If this is an unfinished experiment that is not going to be maintained, tested,
or built, then why is it in the tools directory?

If this is not an experiment, why is it not built?

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:33: import logging
Seriously?

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:42: class FileNotFoundException(Exception):
This seems excessive. The exception is never caught. Why is this not enough for
your purposes:


class Error(Exception):
  pass

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:50: class CommandFailedException(Exception):
Ditto.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:59: HTML = """<!DOCTYPE html>
It is confusing that you have HTML and html variables.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:74: {{script_tags}}
Why not %(script_tags)?

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:105: COMPILATION_ERROR_REGEX =
re.compile(".*legpad compilation error.*", re.DOTALL)
legpad -> dart2js.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:120: # id of script tag that holds name of the
top dart file to be compiled,
Not a proper sentence.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:124: # TODO(mattsh): read this from some config
file once ahe/zundel create it
Not a proper sentence.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:145: "%prog [options] file_to_compile.dart"
Inconsistent formatting of arguments.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:160: sys.exit(1)
I generally dislike constructors that do anything but store their arguments. You
do not expect that creating a new object will exit the process.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:179: self.js_file = self.main_file + ".legpad.js"
I don't like the default of creating "junk" at random places in the source
directory.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:183: # this is the html file that we pass to
DumpRenderTree
Not proper sentence.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:184: self.html_file = self.main_file +
".legpad.html"
Also creates junk in the source directory.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:187: # map from file name to File object
(contains entries for all corelib
Ditto.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:191: # map from script tag id to File object
Not a proper sentence.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:194: self.load_libraries()
I don't think most of the following code belongs in a constructor.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:210: def generate_html(self):
Our style guide says MethodName, not method_name.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:214: tags.append(self._create_tag(MAIN_ID,
self.shorten(self.main_file)))
I thought this was a bug.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:215: html = HTML.replace("{{script_tags}}",
"".join(tags))
Why not:

html = HTML % {'script_tags': "".join(tags)}

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:223: def generate_js(self):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:226: drt += ".app"
This is not enough, please see my previous comments.
client/tests/drt/DumpRenderTree.app is a bundle, which is just a directory
that's handled magically by Finder, but not at the Unix level.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:228: raise Exception("legpad does not run on
Windows")
Actually, you should not use Exception for this. See our style guide.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:242: def _create_tag(id, contents):
_MethodName, and did you mean private or protected here?

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:244: s = s.replace("{{id}}", id)
Why not use Python's % operator?

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:245: # TODO(mattsh) - need to html escape here
Formatting of comment.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:246: s = s.replace("{{contents}}", contents)
%

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:249: def dart_library(self, name):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:252: raise Exception("unrecognized 'dart:%s'",
name)
Don't raise Exception.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:255: def load_libraries(self):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:259: def load_file(self, name):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:266: raise Exception("ambiguous id '%s'" % f.id)
Exception

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:270: def shorten(self, name):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:276: def make_id(self, name):
MethodName.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:281: return self.shorten(name).replace("/",
"_").replace(".", "_")
This doesn't work on Windows. Adding .replace(os.sep, "_") would help.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:290: with open(self.name, "r") as f:
Why don't use read_file below?

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:293: def directives(self):
MethodName.

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:301: def _directive(self, line):
_MethodName. Is this method protected or private?

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:313: def read_file(file_name):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:321: def write_file(file_name, contents):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:329: def check_exists(file_name):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:334: def format_command(args):
MethodName

http://codereview.chromium.org/9866022/diff/16001/tools/testing/legpad/legpad...
tools/testing/legpad/legpad.py:338: def run_command(args):
MethodName