Docserver: Implement ContentProvider.GetVersion.

Docserver: Implement ContentProvider.GetVersion.

BUG=402903
R=yoz@chromium.org
NOTRY=true

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

[Yoyo Zhou, 2014-08-12 22:32:57]

LGTM

https://chromiumcodereview.appspot.com/462103003/diff/1/chrome/common/extensi...
File chrome/common/extensions/docs/server2/content_provider.py (right):

https://chromiumcodereview.appspot.com/462103003/diff/1/chrome/common/extensi...
chrome/common/extensions/docs/server2/content_provider.py:151: return
Future(callback=lambda: stat_future.Get().version)
Is this a Then?

[not at google - send to devlin, 2014-08-13 02:14:29]

I converted every other Get() in this file to be a Then(). Sorry :\ When you
start chaining Futures together... it's hard to stop.

You should look at the diff between the last 2 patches, not the overall diff.

https://codereview.chromium.org/462103003/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/server2/content_provider.py (right):

https://codereview.chromium.org/462103003/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/server2/content_provider.py:151: return
Future(callback=lambda: stat_future.Get().version)
On 2014/08/12 22:32:57, Yoyo Zhou wrote:
> Is this a Then?

ooh, yes it is.

Sorry I went a little overboard here.

[not at google - send to devlin, 2014-08-13 02:14:39]

(PTAL)

[Yoyo Zhou, 2014-08-13 18:30:50]

LGTM

https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
File chrome/common/extensions/docs/server2/content_provider.py (right):

https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
chrome/common/extensions/docs/server2/content_provider.py:173: def
get_first_path_which_exists(existence):
Seems like this wants to be a Race. But maybe it should be deterministic.

[not at google - send to devlin, 2014-08-13 19:38:22]

https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
File chrome/common/extensions/docs/server2/content_provider.py (right):

https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
chrome/common/extensions/docs/server2/content_provider.py:173: def
get_first_path_which_exists(existence):
On 2014/08/13 18:30:50, Yoyo Zhou wrote:
> Seems like this wants to be a Race. But maybe it should be deterministic.

Yeah I wish it could be a Race, but Race returns the first Future which
resolved. There is no way to say "return the first Future which resolves *and*
which matches this condition".

Hm, maybe there should be. You could write this like:

return Race((self.file_system.Exists(path) for path in paths),
            when=lambda exists: exists,  # what to call "when"?
            default=None)  # considering calling default "fallback"

WDYT? I'm not sure how important being deterministic is. I guess we could define
that .html takes precedence over .md or whatever, but I do like using Race here
since that's effectively what it is.

[Yoyo Zhou, 2014-08-13 20:34:15]

https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
File chrome/common/extensions/docs/server2/content_provider.py (right):

https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
chrome/common/extensions/docs/server2/content_provider.py:173: def
get_first_path_which_exists(existence):
On 2014/08/13 19:38:22, kalman wrote:
> On 2014/08/13 18:30:50, Yoyo Zhou wrote:
> > Seems like this wants to be a Race. But maybe it should be deterministic.
> 
> Yeah I wish it could be a Race, but Race returns the first Future which
> resolved. There is no way to say "return the first Future which resolves *and*
> which matches this condition".
> 
> Hm, maybe there should be. You could write this like:
> 
> return Race((self.file_system.Exists(path) for path in paths),
>             when=lambda exists: exists,  # what to call "when"?
>             default=None)  # considering calling default "fallback"
> 
> WDYT? I'm not sure how important being deterministic is. I guess we could
define
> that .html takes precedence over .md or whatever, but I do like using Race
here
> since that's effectively what it is.

Well, you should be able to do this like

Race((self.file_system.Exists(path).Then(raise_exception_if_not_exists) for path
in paths), default=None)

for appropriate values of raise_exception_if_not_exists, right?

[not at google - send to devlin, 2014-08-13 20:39:37]

On 2014/08/13 20:34:15, Yoyo Zhou wrote:
>
https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
> File chrome/common/extensions/docs/server2/content_provider.py (right):
> 
>
https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
> chrome/common/extensions/docs/server2/content_provider.py:173: def
> get_first_path_which_exists(existence):
> On 2014/08/13 19:38:22, kalman wrote:
> > On 2014/08/13 18:30:50, Yoyo Zhou wrote:
> > > Seems like this wants to be a Race. But maybe it should be deterministic.
> > 
> > Yeah I wish it could be a Race, but Race returns the first Future which
> > resolved. There is no way to say "return the first Future which resolves
*and*
> > which matches this condition".
> > 
> > Hm, maybe there should be. You could write this like:
> > 
> > return Race((self.file_system.Exists(path) for path in paths),
> >             when=lambda exists: exists,  # what to call "when"?
> >             default=None)  # considering calling default "fallback"
> > 
> > WDYT? I'm not sure how important being deterministic is. I guess we could
> define
> > that .html takes precedence over .md or whatever, but I do like using Race
> here
> > since that's effectively what it is.
> 
> Well, you should be able to do this like
> 
> Race((self.file_system.Exists(path).Then(raise_exception_if_not_exists) for
path
> in paths), default=None)
> 
> for appropriate values of raise_exception_if_not_exists, right?

Good point, I could simulate this with exceptions. Ok, I'll check that out when
I'm back at my laptop.

[Yoyo Zhou, 2014-08-13 20:44:12]

On 2014/08/13 20:39:37, kalman wrote:
> On 2014/08/13 20:34:15, Yoyo Zhou wrote:
> >
>
https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
> > File chrome/common/extensions/docs/server2/content_provider.py (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/462103003/diff/40001/chrome/common/ext...
> > chrome/common/extensions/docs/server2/content_provider.py:173: def
> > get_first_path_which_exists(existence):
> > On 2014/08/13 19:38:22, kalman wrote:
> > > On 2014/08/13 18:30:50, Yoyo Zhou wrote:
> > > > Seems like this wants to be a Race. But maybe it should be
deterministic.
> > > 
> > > Yeah I wish it could be a Race, but Race returns the first Future which
> > > resolved. There is no way to say "return the first Future which resolves
> *and*
> > > which matches this condition".
> > > 
> > > Hm, maybe there should be. You could write this like:
> > > 
> > > return Race((self.file_system.Exists(path) for path in paths),
> > >             when=lambda exists: exists,  # what to call "when"?
> > >             default=None)  # considering calling default "fallback"
> > > 
> > > WDYT? I'm not sure how important being deterministic is. I guess we could
> > define
> > > that .html takes precedence over .md or whatever, but I do like using Race
> > here
> > > since that's effectively what it is.
> > 
> > Well, you should be able to do this like
> > 
> > Race((self.file_system.Exists(path).Then(raise_exception_if_not_exists) for
> path
> > in paths), default=None)
> > 
> > for appropriate values of raise_exception_if_not_exists, right?
> 
> Good point, I could simulate this with exceptions. Ok, I'll check that out
when
> I'm back at my laptop.

On the other hand, it might not be any easier to read once you've done so.

[not at google - send to devlin, 2014-08-13 20:46:31]

> On the other hand, it might not be any easier to read once you've done so.

Fair nuff. I'll cut my losses and submit this as-is.

[not at google - send to devlin, 2014-08-13 20:46:48]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-08-13 20:49:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/462103003/40001

[commit-bot: I haz the power, 2014-08-13 20:54:53]

Committed patchset #2 (40001) as 289373