Docserver: Add 'deprecated since' message for API nodes

Docserver: Add 'deprecated since' message for API nodes

BUG=392319
NOTRY=True

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

[ahernandez, 2014-07-10 00:53:44]

Since every key in the schema is treated as a node by availability finder, I
just checked the annotation of a node's deprecated key, since that would be the
earliest version at which a node became deprecated.

[not at google - send to devlin, 2014-07-10 16:28:48]

test?

https://codereview.chromium.org/386443003/diff/40001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/386443003/diff/40001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:185: except
AssertionError:
Catching AssertionError is quite dangerous, because there's no saying what the
assertion actually was. You should plumb through that flag to where it's
actually needed.

And "no_assert" isn't the best name... what are you actually disabling
assertions for? I.e. what condition are you allowing that isn't usually allowed?
Call the flag that.

[ahernandez, 2014-07-10 23:18:29]

Added in a test (and a file I forgot to git add earlier). PTAL

[not at google - send to devlin, 2014-07-15 21:51:15]

https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:224: # unnecessary
AssertionErrors.
comment is unnecessary, code is clear enough

https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:240: # Check this
before checking parent availability; we always want
I'd rather that the callers worry about this rather than GetAvailability(). See
below.

https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:584: def
_GetAvailabilityTemplate(self, status=None, version=None, scheduled=None):
a thought: does the function/event level availability take into account / set
scheduled? because the deprecated template should as well, and it should always
have a scheduled time (I don't think it's possible otherwise). I mention this
because while we might render availability as "available in dev channel
(scheduled for chrome 38)" we shouldn't say "deprecated in dev channel" we
should say "deprecated in chrome 38".

https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:589: availability_info
= self._current_node.GetAvailability()
put that logic down here:

availability_info = self._current_node.GetDeprecated()
if availability_info is None:
  availability_info = self._current_node.GetAvailability()
...

or whatever.

[ahernandez, 2014-07-15 22:09:51]

https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
chrome/common/extensions/docs/server2/api_data_source.py:584: def
_GetAvailabilityTemplate(self, status=None, version=None, scheduled=None):
On 2014/07/15 21:51:15, kalman wrote:
> a thought: does the function/event level availability take into account / set
> scheduled? because the deprecated template should as well, and it should
always
> have a scheduled time (I don't think it's possible otherwise). I mention this
> because while we might render availability as "available in dev channel
> (scheduled for chrome 38)" we shouldn't say "deprecated in dev channel" we
> should say "deprecated in chrome 38".

It can't say "deprecated in dev channel"; deprecated_message.html only uses the
ChannelInfo.version attribute, which is always a number.

Deprecated nodes can be scheduled?

[not at google - send to devlin, 2014-07-15 22:37:40]

On 2014/07/15 22:09:51, ahernandez wrote:
>
https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
> File chrome/common/extensions/docs/server2/api_data_source.py (right):
> 
>
https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
> chrome/common/extensions/docs/server2/api_data_source.py:584: def
> _GetAvailabilityTemplate(self, status=None, version=None, scheduled=None):
> On 2014/07/15 21:51:15, kalman wrote:
> > a thought: does the function/event level availability take into account /
set
> > scheduled? because the deprecated template should as well, and it should
> always
> > have a scheduled time (I don't think it's possible otherwise). I mention
this
> > because while we might render availability as "available in dev channel
> > (scheduled for chrome 38)" we shouldn't say "deprecated in dev channel" we
> > should say "deprecated in chrome 38".
> 
> It can't say "deprecated in dev channel"; deprecated_message.html only uses
the
> ChannelInfo.version attribute, which is always a number.
> 
> Deprecated nodes can be scheduled?

Not quite what I meant. It sounds like the situation at the moment is perfect,
"deprecated in chrome N" is all you really need to see. what I mean is that
deprecated nodes *can't* be scheduled, they're by definition always scheduled
(since you can't pin them to a specific chrome version, at least not now).

the question about object level availability is a separate one, they *can* be
scheduled but I don't see the code for that. might just be missing it.

[ahernandez, 2014-07-16 21:27:45]

On 2014/07/15 22:37:40, kalman wrote:
> On 2014/07/15 22:09:51, ahernandez wrote:
> >
>
https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
> > File chrome/common/extensions/docs/server2/api_data_source.py (right):
> > 
> >
>
https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
> > chrome/common/extensions/docs/server2/api_data_source.py:584: def
> > _GetAvailabilityTemplate(self, status=None, version=None, scheduled=None):
> > On 2014/07/15 21:51:15, kalman wrote:
> > > a thought: does the function/event level availability take into account /
> set
> > > scheduled? because the deprecated template should as well, and it should
> > always
> > > have a scheduled time (I don't think it's possible otherwise). I mention
> this
> > > because while we might render availability as "available in dev channel
> > > (scheduled for chrome 38)" we shouldn't say "deprecated in dev channel" we
> > > should say "deprecated in chrome 38".
> > 
> > It can't say "deprecated in dev channel"; deprecated_message.html only uses
> the
> > ChannelInfo.version attribute, which is always a number.
> > 
> > Deprecated nodes can be scheduled?
> 
> Not quite what I meant. It sounds like the situation at the moment is perfect,
> "deprecated in chrome N" is all you really need to see. what I mean is that
> deprecated nodes *can't* be scheduled, they're by definition always scheduled
> (since you can't pin them to a specific chrome version, at least not now).
> 
> the question about object level availability is a separate one, they *can* be
> scheduled but I don't see the code for that. might just be missing it.

Are scheduled nodes documented in the features files the same way scheduled APIs
are? Also, you said deprecated nodes can't be pinned to a specific chrome
version. Why not? I was under the impression that they could.

[not at google - send to devlin, 2014-07-16 22:23:23]

On 2014/07/16 21:27:45, ahernandez wrote:
> On 2014/07/15 22:37:40, kalman wrote:
> > On 2014/07/15 22:09:51, ahernandez wrote:
> > >
> >
>
https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
> > > File chrome/common/extensions/docs/server2/api_data_source.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/386443003/diff/80001/chrome/common/extensions...
> > > chrome/common/extensions/docs/server2/api_data_source.py:584: def
> > > _GetAvailabilityTemplate(self, status=None, version=None, scheduled=None):
> > > On 2014/07/15 21:51:15, kalman wrote:
> > > > a thought: does the function/event level availability take into account
/
> > set
> > > > scheduled? because the deprecated template should as well, and it should
> > > always
> > > > have a scheduled time (I don't think it's possible otherwise). I mention
> > this
> > > > because while we might render availability as "available in dev channel
> > > > (scheduled for chrome 38)" we shouldn't say "deprecated in dev channel"
we
> > > > should say "deprecated in chrome 38".
> > > 
> > > It can't say "deprecated in dev channel"; deprecated_message.html only
uses
> > the
> > > ChannelInfo.version attribute, which is always a number.
> > > 
> > > Deprecated nodes can be scheduled?
> > 
> > Not quite what I meant. It sounds like the situation at the moment is
perfect,
> > "deprecated in chrome N" is all you really need to see. what I mean is that
> > deprecated nodes *can't* be scheduled, they're by definition always
scheduled
> > (since you can't pin them to a specific chrome version, at least not now).
> > 
> > the question about object level availability is a separate one, they *can*
be
> > scheduled but I don't see the code for that. might just be missing it.
> 
> Are scheduled nodes documented in the features files the same way scheduled
APIs
> are? Also, you said deprecated nodes can't be pinned to a specific chrome
> version. Why not? I was under the impression that they could.

I thought you implemented the scheduled part? Anyway - the scheduled logic is
just that if an API is stable in the features file but hasn't reached the stable
channel we render "scheduled for". Since there is no explicit channel info
associated with deprecating nodes, since they're just annotations in JSON files,
deprecated APIs are always implicitly stable, but may have not actually reached
the stable channel yet.

I suppose you could render "Deprecated in Dev channel (scheduled for chrome 38)"
but I think that looks weird.

[ahernandez, 2014-07-17 20:06:55]

PTAL.

[not at google - send to devlin, 2014-07-17 20:30:06]

the important question is the "wait when does this logic kick in"

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_data_source.py:224: return
self._CheckEventCallback(deprecated_path)
'return None' at end.

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_data_source.py:571: def
_GetAvailabilityTemplate(self, status=None, version=None, scheduled=None):
mm I didn't notice these arguments here, with the status-None, version=None,
scheduled=None stuff. I'd rather these arguments weren't here, and the place
that they're used just generate that structure itself, perhaps with a second
helper function.

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_data_source.py:576: availability_info
= self._current_node.GetDeprecated()
please add a comment like 'Displaying deprecated status takes precedence over
when the API became stable'.

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:382: # Check if any
top-level nodes are scheduled.
wait when does this logic kick in?

[ahernandez, 2014-07-17 20:35:24]

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:382: # Check if any
top-level nodes are scheduled.
On 2014/07/17 20:30:06, kalman wrote:
> wait when does this logic kick in?

This happens right after the graph is constructed. I wasn't sure where else I
could fit in the scheduling logic, and right after graph creation seemed
reasonable.

[not at google - send to devlin, 2014-07-17 20:47:21]

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:382: # Check if any
top-level nodes are scheduled.
On 2014/07/17 20:35:24, ahernandez wrote:
> On 2014/07/17 20:30:06, kalman wrote:
> > wait when does this logic kick in?
> 
> This happens right after the graph is constructed. I wasn't sure where else I
> could fit in the scheduling logic, and right after graph creation seemed
> reasonable.

I guess i mean why do you need to add this now

[ahernandez, 2014-07-17 20:50:01]

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:382: # Check if any
top-level nodes are scheduled.
On 2014/07/17 20:47:21, kalman wrote:
> On 2014/07/17 20:35:24, ahernandez wrote:
> > On 2014/07/17 20:30:06, kalman wrote:
> > > wait when does this logic kick in?
> > 
> > This happens right after the graph is constructed. I wasn't sure where else
I
> > could fit in the scheduling logic, and right after graph creation seemed
> > reasonable.
> 
> I guess i mean why do you need to add this now

Oh, did you want that in a separate patch? That probably makes more sense.

[not at google - send to devlin, 2014-07-17 20:57:31]

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
File chrome/common/extensions/docs/server2/availability_finder.py (right):

https://codereview.chromium.org/386443003/diff/120001/chrome/common/extension...
chrome/common/extensions/docs/server2/availability_finder.py:382: # Check if any
top-level nodes are scheduled.
On 2014/07/17 20:50:01, ahernandez wrote:
> On 2014/07/17 20:47:21, kalman wrote:
> > On 2014/07/17 20:35:24, ahernandez wrote:
> > > On 2014/07/17 20:30:06, kalman wrote:
> > > > wait when does this logic kick in?
> > > 
> > > This happens right after the graph is constructed. I wasn't sure where
else
> I
> > > could fit in the scheduling logic, and right after graph creation seemed
> > > reasonable.
> > 
> > I guess i mean why do you need to add this now
> 
> Oh, did you want that in a separate patch? That probably makes more sense.

If you don't need it for this patch then yeah.

[ahernandez, 2014-07-17 21:37:32]

I got rid of the scheduling logic and addressed the other comments, PTAL.

[not at google - send to devlin, 2014-07-17 21:39:22]

lgtm

https://codereview.chromium.org/386443003/diff/140001/chrome/common/extension...
File chrome/common/extensions/docs/server2/api_data_source.py (right):

https://codereview.chromium.org/386443003/diff/140001/chrome/common/extension...
chrome/common/extensions/docs/server2/api_data_source.py:598: version =
availability_info.channel_info.version
inline |scheduled| and |version| ?

[ahernandez, 2014-07-17 22:00:28]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-07-17 22:01:43]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/38644300...

[commit-bot: I haz the power, 2014-07-17 22:04:32]

Change committed as 283899