Minor improvement to File diagnostics

Minor improvement to File diagnostics

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

[Mads Ager (google), 2012-09-27 16:38:32]

LGTM, thanks Peter!

[ahe, 2012-09-27 16:49:48]

On 2012/09/27 16:38:32, Mads Ager wrote:
> LGTM, thanks Peter!

I'll create a test case before submitting.

[Mads Ager (google), 2012-09-27 17:05:02]

On 2012/09/27 16:49:48, ahe wrote:
> On 2012/09/27 16:38:32, Mads Ager wrote:
> > LGTM, thanks Peter!
> 
> I'll create a test case before submitting.

Sounds good, thanks. Also, for other exceptions we have used FileIOException
here. It would make sense to move to ArgumentError and friends. For this change,
maybe you should stick to a FileIOException and we can do a sweeping round of
Exception/Error cleanup of dart:io.

[gram, 2012-09-27 17:06:50]

lgtm

Excellent! Thanks Peter.

[ahe, 2012-09-28 09:15:11]

On 2012/09/27 17:05:02, Mads Ager wrote:
> Sounds good, thanks. Also, for other exceptions we have used FileIOException
> here. It would make sense to move to ArgumentError and friends. For this
change,
> maybe you should stick to a FileIOException and we can do a sweeping round of
> Exception/Error cleanup of dart:io.

I'm not sure you have been consistent about FileIOException, but I'll be happy
to change it if you insist.

Please take a look at the test I added.

[Mads Ager (google), 2012-09-28 09:48:56]

LGTM!

Thanks for adding the test and I was wrong about the consistency of using
FileIOException, so please leave this as an ArgumentError. :)

http://codereview.chromium.org/10983066/diff/2002/dart/tests/standalone/io/fi...
File dart/tests/standalone/io/file_constructor_test.dart (right):

http://codereview.chromium.org/10983066/diff/2002/dart/tests/standalone/io/fi...
dart/tests/standalone/io/file_constructor_test.dart:10: new File('blåbærgrød');
This line and the next should be removed.

[ahe, 2012-09-28 10:42:57]

http://codereview.chromium.org/10983066/diff/2002/dart/tests/standalone/io/fi...
File dart/tests/standalone/io/file_constructor_test.dart (right):

http://codereview.chromium.org/10983066/diff/2002/dart/tests/standalone/io/fi...
dart/tests/standalone/io/file_constructor_test.dart:10: new File('blåbærgrød');
On 2012/09/28 09:48:56, Mads Ager wrote:
> This line and the next should be removed.

Why?

[Mads Ager (google), 2012-09-28 11:05:42]

http://codereview.chromium.org/10983066/diff/2002/dart/tests/standalone/io/fi...
File dart/tests/standalone/io/file_constructor_test.dart (right):

http://codereview.chromium.org/10983066/diff/2002/dart/tests/standalone/io/fi...
dart/tests/standalone/io/file_constructor_test.dart:10: new File('blåbærgrød');
On 2012/09/28 10:42:57, ahe wrote:
> On 2012/09/28 09:48:56, Mads Ager wrote:
> > This line and the next should be removed.
> 
> Why?

I was thinking that it was redundant because of all the other tests that we have
that only work if the File constructor with a string works. But you are right
that this is a good unit test of only the constructor so feel free to keep.

[Ivan Posva, 2012-10-02 00:19:38]

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
File dart/runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
dart/runtime/bin/file_impl.dart:498: throw new
ArgumentError('${NoSuchMethodError.safeToString(_name)} '
Peter can you explain why you are using NoSuchMethodError.safeToString here? I
understand how it can make sense in NoSuchMethodError handling, but here not so.

[ahe, 2012-10-02 04:49:23]

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
File dart/runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
dart/runtime/bin/file_impl.dart:498: throw new
ArgumentError('${NoSuchMethodError.safeToString(_name)} '
On 2012/10/02 00:19:38, Ivan Posva wrote:
> Peter can you explain why you are using NoSuchMethodError.safeToString here? I
> understand how it can make sense in NoSuchMethodError handling, but here not
so.

It makes sense to use safeToString here because the error is about the type of
the object. Generally, calling toString during error reporting is undesirable
for the same reasons as in NoSuchMethodError.

[Ivan Posva, 2012-10-02 05:19:11]

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
File dart/runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
dart/runtime/bin/file_impl.dart:498: throw new
ArgumentError('${NoSuchMethodError.safeToString(_name)} '
On 2012/10/02 04:49:24, ahe wrote:
> On 2012/10/02 00:19:38, Ivan Posva wrote:
> > Peter can you explain why you are using NoSuchMethodError.safeToString here?
I
> > understand how it can make sense in NoSuchMethodError handling, but here not
> so.
> 
> It makes sense to use safeToString here because the error is about the type of
> the object. Generally, calling toString during error reporting is undesirable
> for the same reasons as in NoSuchMethodError. 

It just feels very strange to be calling some random static method of some
exception here.

[ahe, 2012-10-02 05:39:46]

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
File dart/runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
dart/runtime/bin/file_impl.dart:498: throw new
ArgumentError('${NoSuchMethodError.safeToString(_name)} '
On 2012/10/02 05:19:11, Ivan Posva wrote:
> On 2012/10/02 04:49:24, ahe wrote:
> > On 2012/10/02 00:19:38, Ivan Posva wrote:
> > > Peter can you explain why you are using NoSuchMethodError.safeToString
here?
> I
> > > understand how it can make sense in NoSuchMethodError handling, but here
not
> > so.
> > 
> > It makes sense to use safeToString here because the error is about the type
of
> > the object. Generally, calling toString during error reporting is
undesirable
> > for the same reasons as in NoSuchMethodError. 
> 
> It just feels very strange to be calling some random static method of some
> exception here.

Agreed, I would like to find a better place for the static method, and perhaps
make it more convenient to use. I'll file a bug.

[ahe, 2012-10-02 07:47:11]

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
File dart/runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/10983066/diff/1/dart/runtime/bin/file_...
dart/runtime/bin/file_impl.dart:498: throw new
ArgumentError('${NoSuchMethodError.safeToString(_name)} '
On 2012/10/02 05:39:47, ahe wrote:
> On 2012/10/02 05:19:11, Ivan Posva wrote:
> > On 2012/10/02 04:49:24, ahe wrote:
> > > On 2012/10/02 00:19:38, Ivan Posva wrote:
> > > > Peter can you explain why you are using NoSuchMethodError.safeToString
> here?
> > I
> > > > understand how it can make sense in NoSuchMethodError handling, but here
> not
> > > so.
> > > 
> > > It makes sense to use safeToString here because the error is about the
type
> of
> > > the object. Generally, calling toString during error reporting is
> undesirable
> > > for the same reasons as in NoSuchMethodError. 
> > 
> > It just feels very strange to be calling some random static method of some
> > exception here.
> 
> Agreed, I would like to find a better place for the static method, and perhaps
> make it more convenient to use. I'll file a bug. 

Filed http://dartbug.com/5596.