Extensions Docs Server: Uniform handling of file not found errors

Extensions Docs Server: Uniform handling of file not found errors

All the FileSystem implementations now handle missing files the same way (by
throwing a FileNotFoundError).

BUG=141664
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151078

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151319

[not at google - send to devlin, 2012-08-10 06:12:16]

lgtm

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/api_data_source.py:98: path)
not sure, perhaps this should also be catching FileNotFoundError and return
None. It matches how the other data sources work.

(log error though)

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/api_list_data_source.py:56: except
FileNotFoundError:
log error?

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/intro_data_source.py:83: return None
log error before returning None?

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/local_file_system.py (right):

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/local_file_system.py:27: except IOError:
why IOError here and OSError other times? Python quirk?

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/subversion_file_system.py:23: return
self.StatInfo(int(re.search('([0-9]+)', dir_html).group(0)))
also need to do something here too?

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
File
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html
(right):

http://codereview.chromium.org/10830252/diff/1/chrome/common/extensions/docs/...
chrome/common/extensions/docs/server2/templates/private/standard_apps_api.html:26:
{{+intro.intro is_apps:true}}
this, OTOH, is a good use for is_apps. I think.

[cduvall, 2012-08-10 17:25:16]

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:98: path)
On 2012/08/10 06:12:16, kalman wrote:
> not sure, perhaps this should also be catching FileNotFoundError and return
> None. It matches how the other data sources work.
> 
> (log error though)

I would rather have it throw the exception than return None, so we actually know
when when it fails. Otherwise it will pass the integration test even if there is
no JSON/IDL schema.

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_list_data_source.py:56: except
FileNotFoundError:
On 2012/08/10 06:12:16, kalman wrote:
> log error?

Done.

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/intro_data_source.py:83: return None
On 2012/08/10 06:12:16, kalman wrote:
> log error before returning None?

Done.

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/local_file_system.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/local_file_system.py:27: except IOError:
On 2012/08/10 06:12:16, kalman wrote:
> why IOError here and OSError other times? Python quirk?

Yep, os.listdir throws OSError, but open throws IOError.

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/subversion_file_system.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/subversion_file_system.py:23: return
self.StatInfo(int(re.search('([0-9]+)', dir_html).group(0)))
On 2012/08/10 06:12:16, kalman wrote:
> also need to do something here too?

Done.

[not at google - send to devlin, 2012-08-12 22:22:58]

Need to tweak that error throwing, lgtm after that.

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:98: path)
On 2012/08/10 17:25:16, cduvall wrote:
> On 2012/08/10 06:12:16, kalman wrote:
> > not sure, perhaps this should also be catching FileNotFoundError and return
> > None. It matches how the other data sources work.
> > 
> > (log error though)
> 
> I would rather have it throw the exception than return None, so we actually
know
> when when it fails. Otherwise it will pass the integration test even if there
is
> no JSON/IDL schema.

Ok sg. Makes me wonder if there are other things (like, some of those other
handlers that return None rather than raising exceptions) that the integration
test won't pick up.

But yeah, no point logging if you're just raising it again anyway. So... back to
how it was?

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_list_data_source.py:56: except
FileNotFoundError:
On 2012/08/10 17:25:16, cduvall wrote:
> On 2012/08/10 06:12:16, kalman wrote:
> > log error?
> 
> Done.

By the same logic as ApiDataSource we shouldn't catch this error here. Let it be
an error so that the integration tests can catch it.

Though in this case I'm not sure FileNotFoundError is the correct error to
raise. The apps/extensions directory not being present would imply a programming
error (since we're controlling the data that calls this). So perhaps turn this
error into a ValueError (with an appropriate message, and constructed with the
old error of course... presuming that's possible).

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/intro_data_source.py:83: return None
On 2012/08/10 17:25:16, cduvall wrote:
> On 2012/08/10 06:12:16, kalman wrote:
> > log error before returning None?
> 
> Done.

Like ApiListDataSource, this should only be happening if we make a mistake
writing the templates. So rather than logging and returning None, throw a
ValueError if it isn't found?

Btw by "logging error" here I mean like logging.error("No intro found"). Logging
the last error might be misleading.

... though should raise ValueError("No intro found for \"%s\"" % key) anyway.

[cduvall, 2012-08-13 18:44:05]

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_data_source.py:98: path)
On 2012/08/12 22:22:58, kalman wrote:
> On 2012/08/10 17:25:16, cduvall wrote:
> > On 2012/08/10 06:12:16, kalman wrote:
> > > not sure, perhaps this should also be catching FileNotFoundError and
return
> > > None. It matches how the other data sources work.
> > > 
> > > (log error though)
> > 
> > I would rather have it throw the exception than return None, so we actually
> know
> > when when it fails. Otherwise it will pass the integration test even if
there
> is
> > no JSON/IDL schema.
> 
> Ok sg. Makes me wonder if there are other things (like, some of those other
> handlers that return None rather than raising exceptions) that the integration
> test won't pick up.
> 
> But yeah, no point logging if you're just raising it again anyway. So... back
to
> how it was?

Done.

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/api_list_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/api_list_data_source.py:56: except
FileNotFoundError:
On 2012/08/12 22:22:58, kalman wrote:
> On 2012/08/10 17:25:16, cduvall wrote:
> > On 2012/08/10 06:12:16, kalman wrote:
> > > log error?
> > 
> > Done.
> 
> By the same logic as ApiDataSource we shouldn't catch this error here. Let it
be
> an error so that the integration tests can catch it.
> 
> Though in this case I'm not sure FileNotFoundError is the correct error to
> raise. The apps/extensions directory not being present would imply a
programming
> error (since we're controlling the data that calls this). So perhaps turn this
> error into a ValueError (with an appropriate message, and constructed with the
> old error of course... presuming that's possible).

Used the error as a string plus a more specific message, I think that's the best
way to do it in python.

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
File chrome/common/extensions/docs/server2/intro_data_source.py (right):

https://chromiumcodereview.appspot.com/10830252/diff/1/chrome/common/extensio...
chrome/common/extensions/docs/server2/intro_data_source.py:83: return None
On 2012/08/12 22:22:58, kalman wrote:
> On 2012/08/10 17:25:16, cduvall wrote:
> > On 2012/08/10 06:12:16, kalman wrote:
> > > log error before returning None?
> > 
> > Done.
> 
> Like ApiListDataSource, this should only be happening if we make a mistake
> writing the templates. So rather than logging and returning None, throw a
> ValueError if it isn't found?
> 
> Btw by "logging error" here I mean like logging.error("No intro found").
Logging
> the last error might be misleading.
> 
> ... though should raise ValueError("No intro found for \"%s\"" % key) anyway.

Done.