Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14322003:
Inline docs render properly in extensions doc server. (Closed)
|
Created:
7 years, 8 months ago by jshumway Modified:
7 years, 8 months ago CC:
chromium-reviews, benjhayden+dwatch_chromium.org, Aaron Boodman, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionInline docs render properly in extensions doc server.
Documents in idl files with the [inline_doc] attribute are now properly rendered inline. Links to inline docs no longer work and will cause errors.
BUG=187494
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196593
Patch Set 1 #
Total comments: 7
Patch Set 2 : Codereview changes #Patch Set 3 : Inline doc fixes #Patch Set 4 : Merged with master #
Total comments: 4
Patch Set 5 : Comment and whitespace fixes #
Total comments: 12
Patch Set 6 : Display and test changes #
Total comments: 9
Patch Set 7 : Better test and inline doc function #
Total comments: 2
Patch Set 8 : Small test changes #
Messages
Total messages: 29 (0 generated)
Would you mind staging them? pushd chrome/common/extensions/docs/server2; (python preview.py &>/dev/null &);popd
https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/downloads.idl:35: // The action to take if $ref:FilenameSuggestion.filename already exists. <code>filename</code>
We have the patch running here: http://chrome-apps-doc2.appspot.com/extensions/ There are many inline documents on this page: http://chrome-apps-doc2.appspot.com/extensions/downloads.html
https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_data_source.py:41: """ Replace '$ref's that refer to inline_docs with the json for those docs no space after """ https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_data_source_test.py:142: with open(os.path.join(sys.path[0], 'test_data/test_idl/inline.idl')) as f: I would make a _ReadLocalIDLFile and _ReadLocalJSONFile (like the current _ReadLocalFile) and use that here. https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/test_data/test_idl/inline.idl (right): https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/test_data/test_idl/inline.idl:21: static void requestKeepAwake(Level level); Can you also test an inline ref nested within a parameter object?
Did something happen to onDeterminingFilename? I don't see it on chrome-apps-doc2.
https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/downloads.idl:35: // The action to take if $ref:FilenameSuggestion.filename already exists. On 2013/04/17 13:28:18, benjhayden_chromium wrote: > <code>filename</code> Got the replacement and another. I wonder why these did not cause an error during the ref look up. https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/server2/api_data_source_test.py:142: with open(os.path.join(sys.path[0], 'test_data/test_idl/inline.idl')) as f: On 2013/04/18 00:02:41, cduvall wrote: > I would make a _ReadLocalIDLFile and _ReadLocalJSONFile (like the current > _ReadLocalFile) and use that here. Generalized read local file with a default argument test_dir to work for both IDL and JSON files.
On 2013/04/18 01:07:09, benjhayden_chromium wrote: > Did something happen to onDeterminingFilename? I don't see it on > chrome-apps-doc2. It is only in the Trunk documentation: http://chrome-apps-doc2.appspot.com/trunk/extensions/downloads.html
https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/14322003/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/downloads.idl:35: // The action to take if $ref:FilenameSuggestion.filename already exists. On 2013/04/18 01:14:19, jaredshumway94 wrote: > On 2013/04/17 13:28:18, benjhayden_chromium wrote: > > <code>filename</code> > > Got the replacement and another. I wonder why these did not cause an error > during the ref look up. It might be good to figure this out before committing so that this kind of error doesn't creep back in when people like me edit the IDLs. Does the ref lookup run before or after _InlineDocs? Should _InlineDocs remove the inlined type from the schema when it replaces its uses so that ref lookup can't find it?
Good point. Turns out that the error was not being raised because descriptions were being overwritten when a doc was inlined. This is because in two places enums in the idl file had their description at html. This chunk of html replaced the descriptions that would have caused an error. So I fixed that. This also caused me to spot another error: when a doc is inlined, its type will never be shown. I added some code to show type annotations for inlined docs to fix this. Ref lookup happens after _InlineDocs. Unfortunately, because one document can be inlined more than once, we can no longer link to them. If we gave inlined docs an id then linked to them anyway, sometimes clicking a link in one section of the document would jump you way up the page to a different occurrence of the same inlined doc.
https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:39: # Gather the types with inline_doc. This comment confuses me. This is gathering all types, not just inline_doc ones right? Maybe I would be less confused if you moved it down to line 47 https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:127: # inline put a '.' at the end of all comments.
Removed some newlines. https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:39: # Gather the types with inline_doc. On 2013/04/24 00:09:54, cduvall wrote: > This comment confuses me. This is gathering all types, not just inline_doc ones > right? Maybe I would be less confused if you moved it down to line 47 Did that https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/24001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:127: # inline On 2013/04/24 00:09:54, cduvall wrote: > put a '.' at the end of all comments. Usually do, must have been a typo.
lgtm
+kalman ptal when you have a chance
Is there any way to test that references to inlined types throw errors? That test would be very nice to have, but I won't hold the review for it. LGTM
https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:28: [inline_doc] dictionary FilenameSuggestion { let's just remove the [inline_doc] from here, it looks like a bug, since these <code>FilenameSuggestion</code> references don't make sense without the type being defined. benjhayden: wdyt? https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:44: return move this up under where types is assigned https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:62: node['extended_description'] = inline_docs[ref].pop('description') Hm, very interesting. I hadn't thought of that. Now I have, and I have a proposal. Let's just ignore the inlined type's description. Inlining the docs implies that the type isn't valuable as an entity by itself, which implies that any comment made as though it were isn't valuable either. The bonus of that is that we don't need extended_description. https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:135: self.assertEqual(True, False); Throwing an error will fail the test anyway... https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:136: I think it would be more valuable to test a JSON structure then assert that inlining its docs is what you expect. Like with the testRemoveNoDocs method. IDL is hard to test since it's not clearly defined what the JSON output should look like, so you're forced to do vague testing like here, which isn't that valuable. Hard to see where it goes wrong, won't catch all errors, etc. https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/private/property.html (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/templates/private/property.html:6: {{?inline_type}}<code>{{{inline_type}}}</code>{{/inline_type}} I don't think this is necessary, it's unlikely that developers care what the inlined type was called.
https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:28: [inline_doc] dictionary FilenameSuggestion { On 2013/04/24 19:36:11, kalman wrote: > let's just remove the [inline_doc] from here, it looks like a bug, since these > <code>FilenameSuggestion</code> references don't make sense without the type > being defined. > > benjhayden: wdyt? http://chrome-apps-doc2.appspot.com/trunk/extensions/downloads.html Counter-offer: * keep it [inline_doc]. * Remove the comment above 'dictionary FilenameSuggestion {' which is not displayed. * Change all <code>FilenameSuggestion</code> to <code>suggestion</code>. * Change <code>FilenameSuggestion.filename</code> to just <code>filename</code>. wdyt?
whoa what is going on with those docs; suggest a function?
Did some part of this change already go in? I am so confused. That doc looks half inlined already.
On 2013/04/24 20:14:20, kalman wrote: > Did some part of this change already go in? I am so confused. That doc looks > half inlined already. I think an earlier version of this CL was pushed to chrome-apps-doc2 as a staging ground, not actual live production.
oh right. 2. ok :)
https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:44: return On 2013/04/24 19:36:11, kalman wrote: > move this up under where types is assigned done https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:62: node['extended_description'] = inline_docs[ref].pop('description') On 2013/04/24 19:36:11, kalman wrote: > Hm, very interesting. I hadn't thought of that. > > Now I have, and I have a proposal. Let's just ignore the inlined type's > description. Inlining the docs implies that the type isn't valuable as an entity > by itself, which implies that any comment made as though it were isn't valuable > either. > > The bonus of that is that we don't need extended_description. Okay, I removed both inline_type and extended_description. https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:135: self.assertEqual(True, False); On 2013/04/24 19:36:11, kalman wrote: > Throwing an error will fail the test anyway... not really sure what I was thinking there, fixed. https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:136: On 2013/04/24 19:36:11, kalman wrote: > I think it would be more valuable to test a JSON structure then assert that > inlining its docs is what you expect. Like with the testRemoveNoDocs method. > > IDL is hard to test since it's not clearly defined what the JSON output should > look like, so you're forced to do vague testing like here, which isn't that > valuable. Hard to see where it goes wrong, won't catch all errors, etc. I have made it so https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/private/property.html (right): https://codereview.chromium.org/14322003/diff/28001/chrome/common/extensions/... chrome/common/extensions/docs/templates/private/property.html:6: {{?inline_type}}<code>{{{inline_type}}}</code>{{/inline_type}} On 2013/04/24 19:36:11, kalman wrote: > I don't think this is necessary, it's unlikely that developers care what the > inlined type was called. inlined_type has been removed. I believe this brings json_schema_complier/model.py back to its state before this issue.
cool https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... File chrome/common/extensions/api/downloads.idl (right): https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/api/downloads.idl:44: DOMString url; I'll let benjhayden decide what to do with this file, but like I said, it can't have <code></code> around things that don't exist any more than it can $ref them. https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:18: if you sync (maybe you already have) the version stuff isn't needed anymore. https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:51: del type_['id'] you'll need to delete the description too? https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:66: if isinstance(i, collections.Mapping): maybe this function should be rotated a bit so that the isinstance check only appears once, at the top. def apply_inline(node): if not isinstance(node, collections.Mapping): return .. etc https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:123: self.assertEqual(self._LoadJSON('expected_inline.json'), jsc) still some things missing here e.g. making sure that the "documentation" property isn't taken across, etc. You don't necessarily even need a test file like this, you can construct dicts just as concisely in python. schema = { "namespace": "storage", "types": [{ "id": "Key", "inline_doc": True, "description": "A key", "type": "string" }, { "id": "KeyList", "type": "array", "description": "A list of keys", "items": ["$ref": "Key"] // this will be inlined } "properties": { "key1": {"description": "first key", "$ref": "Key"}, "key2": {"description": "second key", "$ref": "Key"} } } etc. my indentation and stuff is all off there. But run that through _InlineDoc and assert that the transformation is correct.
https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source.py (right): https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:51: del type_['id'] On 2013/04/25 00:19:35, kalman wrote: > you'll need to delete the description too? Got it https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source.py:66: if isinstance(i, collections.Mapping): On 2013/04/25 00:19:35, kalman wrote: > maybe this function should be rotated a bit so that the isinstance check only > appears once, at the top. > > def apply_inline(node): > if not isinstance(node, collections.Mapping): > return > .. etc I did rotate it but two checks are still needed: one to make sure that dict operations are only done to Mappings, and another to recur onto each node in a list, if the current node is a list. https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:22: import third_party.json_schema_compiler.model as model The imports on lines 20 and 21 are no longer needed (see below), and the import on line 22 isn't needed at all. https://codereview.chromium.org/14322003/diff/43001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:123: self.assertEqual(self._LoadJSON('expected_inline.json'), jsc) I see what you mean now. I removed the unnecessary idl and JSC components of this test. Now it is just passing a dictionary through _InlineDocs and testing what comes out. Acordingly, I removed the test_idl folder and expected_inline.json file, and reverted _ReadLocalFile. There are some comments explaining what should happen to the input json where it is declared in the file, I'm not sure if these are extraneous or not.
nice. lgtm when ben is happy with the downloads.idl changes. https://codereview.chromium.org/14322003/diff/49001/chrome/common/extensions/... File chrome/common/extensions/docs/server2/api_data_source_test.py (right): https://codereview.chromium.org/14322003/diff/49001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:149: inlined_correct = { usual terminology in tests is "expected" https://codereview.chromium.org/14322003/diff/49001/chrome/common/extensions/... chrome/common/extensions/docs/server2/api_data_source_test.py:178: self.assertEqual(inlined_docs, inlined_correct) and idiom is to put the expected value first in the argument list.
LGTM Don't worry about downloads.idl, I can clean up the few remaining nits later as long as [inline_doc] is working and tested now. Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jaredshumway94@gmail.com/14322003/53001
Message was sent while issue was closed.
Change committed as 196593 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
