Fixed TODOs/bugs in codegen_tools.dart for code generation of a directory.

Fixed TODOs/bugs in codegen_tools.dart for code generation of a directory.

R=paulberry@google.com

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

[Paul Berry, 2014-08-14 12:44:05]

lgtm with comments addressed.

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_tools.dart (right):

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:404: File outputFile = new
File(joinAll(posix.split(outputDirPath + file)));
Can we change this to:

  File outputFile = new File(joinAll(posix.split(posix.join(outputDirPath,
file))));

That way the correct thing will happen even if outputDirPath doesn't end in '/'.

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:409: } catch (e) {
Let's move the try/catch out so that its scope includes the
outputFile.listSync() call.  That way if the directory doesn't exist the user
will get an error message telling them to run code generation, rather than a
stacktrace.

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:427:
outputFile.deleteSync(recursive: true);
Put a try/catch around this line as well, so that if the directory doesn't exist
at the time generate() is called, we just create it without complaining.

[jwren, 2014-08-14 18:12:29]

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
File pkg/analysis_server/tool/spec/codegen_tools.dart (right):

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:404: File outputFile = new
File(joinAll(posix.split(outputDirPath + file)));
On 2014/08/14 12:44:05, Paul Berry wrote:
> Can we change this to:
> 
>   File outputFile = new File(joinAll(posix.split(posix.join(outputDirPath,
> file))));
> 
> That way the correct thing will happen even if outputDirPath doesn't end in
'/'.

Done.

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:409: } catch (e) {
On 2014/08/14 12:44:05, Paul Berry wrote:
> Let's move the try/catch out so that its scope includes the
> outputFile.listSync() call.  That way if the directory doesn't exist the user
> will get an error message telling them to run code generation, rather than a
> stacktrace.

Done.

https://codereview.chromium.org/476623002/diff/20001/pkg/analysis_server/tool...
pkg/analysis_server/tool/spec/codegen_tools.dart:427:
outputFile.deleteSync(recursive: true);
On 2014/08/14 12:44:05, Paul Berry wrote:
> Put a try/catch around this line as well, so that if the directory doesn't
exist
> at the time generate() is called, we just create it without complaining.

Done.

[jwren, 2014-08-14 18:15:10]

Committed patchset #3 manually as 39258 (presubmit successful).

[Bill Hesse, 2014-08-18 13:48:38]

This additional check fails on svn checkouts using older versions of subversion,
which put a .svn file in each subdirectory.  I think this check should be
modified to ignore files starting with ., or perhaps files not ending in .java.

This is causing failures on the buildbots.  I'll fix them by re-checking out,
using the new svn.

[Bill Hesse, 2014-08-18 14:05:07]

On 2014/08/18 13:48:38, Bill Hesse wrote:
> This additional check fails on svn checkouts using older versions of
subversion,
> which put a .svn file in each subdirectory.  I think this check should be
> modified to ignore files starting with ., or perhaps files not ending in
.java.
> 
> This is causing failures on the buildbots.  I'll fix them by re-checking out,
> using the new svn.

The buildbots do not have svn 1.7, only 1.6, so the problem must be fixed by
ignoring the .svn directory.