Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10201012:
add appcache generation to apidocs (Closed)
|
Created:
8 years, 8 months ago by sethladd Modified:
8 years, 6 months ago Reviewers:
Bob Nystrom CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionadd appcache generation to apidocs
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=7338
Patch Set 1 #Patch Set 2 : enable doc generation again #
Total comments: 8
Patch Set 3 : refactor from review #
Total comments: 26
Patch Set 4 : tweaks from review #Patch Set 5 : tweaks from review #Patch Set 6 : use a specific analytics num #Messages
Total messages: 8 (0 generated)
This fails because minfrog is gone, but I wanted to start the review now. Thanks!
I like that you're adding this, and the general approach, but how about we move it into dartdoc (which possibly a flag to disable it if users don't want it)? https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... File utils/apidoc/apidoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:179: htmlAttributes = 'manifest="appcache.manifest"'; Is there any reason we wouldn't want dartdoc itself to just always do this? Seems generally useful. https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:505: var manifestFile = new File('docs/appcache.manifest'); The Dartdoc class has a more pleasant API for writing files. You should be able to use this here and then do: startFile('docs/appcache.manifest'); writeLine('CACHE MANIFEST\n\n'); ... endFile(); which is a lot more pleasant. https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:516: var relativePath = filename.substring(pathPrefixLength+1); Spaces around + https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:519: toCache.onDone = (done) => writer.close(); Ugh, async. I wonder if at some point, Dartdoc will just have to be rewritten in async style. This code works and is fine, but it relies on the fact that generating the docs themselves is done synchronously so we can rely on the files being written before this starts walking the directory.
please take another look https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... File utils/apidoc/apidoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:179: htmlAttributes = 'manifest="appcache.manifest"'; moved to dartdoc https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:505: var manifestFile = new File('docs/appcache.manifest'); current way works, will leave for now. https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:516: var relativePath = filename.substring(pathPrefixLength+1); On 2012/04/24 16:30:33, Bob Nystrom wrote: > Spaces around + Done. https://chromiumcodereview.appspot.com/10201012/diff/2001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:519: toCache.onDone = (done) => writer.close(); See the added Futures.
Test at http://12.dartlang-api.appspot.com/index.html (open up dev tools and wait for everything to download) If this works, we can add visual clues to let user know that we're loaded up and offline enabled.
I like it. Couple of suggestions. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... File lib/dartdoc/dartdoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:61: bool generateAppCache = false; Instead of setting this to false, just leave it null... https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:115: dartdoc.generateAppCache = generateAppCache; ...and then here, do the if(!= null) check. That way the default value for this is only in the Dartdoc class and not in multiple places. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:121: Future scriptCompiled = compileScript(compilerPath, libDir, Just use "final" here and elsewhere. We don't usually type annotate our locals. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:166: var completer = new Completer(); This codebase generally uses "final" for single assignment locals, so I'd do that here to be consistent. (In the future, I think we may standardize on using "var" everywhere, but that isn't in the style guide yet.) https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:378: var htmlAttributes = generateAppCache ? 'manifest="/appcache.manifest"' : ''; Long line. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:1329: var manifestFile = new File('$outputDir/appcache.manifest'); Can you use the file writing API dartdoc provides here? It's much more pleasant. https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... File utils/apidoc/apidoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:83: Future scriptCompiled = doc.compileScript(compilerPath, libDir, "final" https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:89: Future copiedStatic = doc.copyFiles('${doc.scriptDir}/../../lib/dartdoc/static', outputDir); Long line. https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:92: Future copiedApiDocStatic = doc.copyFiles('${doc.scriptDir}/static', outputDir); Here too. https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:124: // futures are done, but app cache generation is inside document() Why not just have generateAppCache be a separate method on Dartdoc that is invoked directly here separately from document()? Likewise, main() in dartdoc.dart can do the same.
please take another look, thanks! https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... File lib/dartdoc/dartdoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:61: bool generateAppCache = false; OK, but why? In this case, false is the default https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:115: dartdoc.generateAppCache = generateAppCache; On 2012/04/25 21:43:15, Bob Nystrom wrote: > ...and then here, do the if(!= null) check. That way the default value for this > is only in the Dartdoc class and not in multiple places. Done. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:121: Future scriptCompiled = compileScript(compilerPath, libDir, On 2012/04/25 21:43:15, Bob Nystrom wrote: > Just use "final" here and elsewhere. We don't usually type annotate our locals. Done. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:166: var completer = new Completer(); On 2012/04/25 21:43:15, Bob Nystrom wrote: > This codebase generally uses "final" for single assignment locals, so I'd do > that here to be consistent. > > (In the future, I think we may standardize on using "var" everywhere, but that > isn't in the style guide yet.) Done. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:378: var htmlAttributes = generateAppCache ? 'manifest="/appcache.manifest"' : ''; On 2012/04/25 21:43:15, Bob Nystrom wrote: > Long line. Done. https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:1329: var manifestFile = new File('$outputDir/appcache.manifest'); sure, but as a new developer working on this code, that API wasn't obvious. new developers now have to understand a proprietary API. it's also not documented. for instance, endFile() includes the $outputDir. I had to read the method to know that. https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... File utils/apidoc/apidoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:83: Future scriptCompiled = doc.compileScript(compilerPath, libDir, On 2012/04/25 21:43:15, Bob Nystrom wrote: > "final" Done. https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:89: Future copiedStatic = doc.copyFiles('${doc.scriptDir}/../../lib/dartdoc/static', outputDir); On 2012/04/25 21:43:15, Bob Nystrom wrote: > Long line. Done. https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:92: Future copiedApiDocStatic = doc.copyFiles('${doc.scriptDir}/static', outputDir); On 2012/04/25 21:43:15, Bob Nystrom wrote: > Here too. Done. https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:124: // futures are done, but app cache generation is inside document() class Dartdoc still needs the generateAppCache boolean flag, this proposal makes us check for the flag in three places.
A couple of nits, but LGTM! https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... File lib/dartdoc/dartdoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:61: bool generateAppCache = false; On 2012/04/25 23:13:28, sethladd wrote: > OK, but why? In this case, false is the default I try to have a single canonical point of truth for most values. DRY? https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:378: var htmlAttributes = generateAppCache ? 'manifest="/appcache.manifest"' : ''; On 2012/04/25 23:13:28, sethladd wrote: > On 2012/04/25 21:43:15, Bob Nystrom wrote: > > Long line. > > Done. "final" too. :) https://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc... lib/dartdoc/dartdoc.dart:1329: var manifestFile = new File('$outputDir/appcache.manifest'); On 2012/04/25 23:13:28, sethladd wrote: > sure, but as a new developer working on this code, that API wasn't obvious. Well, you do have to learn the codebase you work in at least somewhat. write() etl. al. are used pervasively in this file. > new developers now have to understand a proprietary API. Right, but that's kind of what dartdoc is. It writes files and it has code already to make that easier. Why not use it consistently? >it's also not documented. Sorry. :( I don't see any docs on this method either. ;) I inherited this from a hacked-together sample script so it still needs polish. > > for instance, endFile() includes the $outputDir. I had to read the method to > know that. :( https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... File utils/apidoc/apidoc.dart (right): https://chromiumcodereview.appspot.com/10201012/diff/8001/utils/apidoc/apidoc... utils/apidoc/apidoc.dart:124: // futures are done, but app cache generation is inside document() On 2012/04/25 23:13:28, sethladd wrote: > class Dartdoc still needs the generateAppCache boolean flag, this proposal makes > us check for the flag in three places. Ah, OK.
just cleaning up http://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc.... File lib/dartdoc/dartdoc.dart (right): http://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc.... lib/dartdoc/dartdoc.dart:378: var htmlAttributes = generateAppCache ? 'manifest="/appcache.manifest"' : ''; On 2012/04/25 23:27:50, Bob Nystrom wrote: > On 2012/04/25 23:13:28, sethladd wrote: > > On 2012/04/25 21:43:15, Bob Nystrom wrote: > > > Long line. > > > > Done. > > "final" too. :) Done. http://chromiumcodereview.appspot.com/10201012/diff/8001/lib/dartdoc/dartdoc.... lib/dartdoc/dartdoc.dart:1329: var manifestFile = new File('$outputDir/appcache.manifest'); touché! :) |
