Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
868843002:
net docs: Barebones doc renderer and net_docs target. (Closed)
|
Created:
5 years, 11 months ago by Deprecated (see juliatuttle) Modified:
5 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionnet docs: Barebones doc renderer and net_docs target.
Add a barebones Python script to render network stack documentation
(into $PRODUCT_DIR/net/docs) and a gyp action to call the script.
BUG=451160
Committed: https://crrev.com/a898dd47f3cadba45edc5fa57c9772545bf8b8f2
Cr-Commit-Position: refs/heads/master@{#317825}
Patch Set 1 #Patch Set 2 : Split install-build-deps.sh change into another CL; remove debugging print statement. #
Total comments: 14
Patch Set 3 : Address codereview comments; remove a couple more prints #Patch Set 4 : Dynamically modify sys.path to use third_party/markdown #Patch Set 5 : Add net/sdch/README.md #Patch Set 6 : Add a couple of comments to net_docs.py #
Total comments: 12
Patch Set 7 : Make requested changes #
Total comments: 4
Patch Set 8 : Make requested changes #
Messages
Total messages: 20 (4 generated)
ttuttle@chromium.org changed reviewers: + rdsmith@chromium.org
PTAL, rdsmith.
In reading this CL, I was struck by just how much I didn't know. You really shouldn't rely on me around the GYP stuff, for instance, and I also don't remember much about our python style guide (though I can remedy that, and arguably should :-}). Still, here's a first pass of comments. https://codereview.chromium.org/868843002/diff/20001/net/README.md File net/README.md (right): https://codereview.chromium.org/868843002/diff/20001/net/README.md#newcode1 net/README.md:1: TODO: Add network stack documentation. :) Cute, but I wince at it too; it feels like we shouldn't commit a file that's purely a Todo. Can you write up a bit about what the network stack is, what the important subdirectories are, and what the plan for documentation is, to go in here? https://codereview.chromium.org/868843002/diff/20001/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/868843002/diff/20001/net/net.gypi#newcode1782 net/net.gypi:1782: 'net_docs_output_dir': '<(PRODUCT_DIR)/net/docs', My instinct looking at this file is that it's not the right place for net_docs_script or net_docs_output_dir. But I'm not sure what would be the right place, and I'm not even sure I'm right about this not being the right place :-J. <noideadog/> https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:6: I'm finding myself struck by two things about this file: It's lack of comments/documentation, and it's lack of object orientation. Willing to rework it to remedy those lacks? I'm uncertain about the OO--I don't want to simply assume that OO is always better. But the python scripts I've seen us use in Chrome are pretty consistently OO with a bit of a procedural wrapper at the end for running the script, and being consistent is good :-}. It does allow for composition in the future (easier to turn the file into a module or add stuff without worrying about namespace collisions). And OO does allow more natural places for documentation (class doc and method doc). My thought is having an object per markdown file, and having all the work done in methods on that object. But my concern is with the OO. https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:7: nit: extra blank line. https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:31: TEMPLATE = '<html><head><title>%s</title></head><body>%s</body></html>' Suggestion: Pull this out of the function and make it a multi-line string, just to make it easier to read and modify separately.
Thanks for your comments, rdsmith. https://codereview.chromium.org/868843002/diff/20001/net/README.md File net/README.md (right): https://codereview.chromium.org/868843002/diff/20001/net/README.md#newcode1 net/README.md:1: TODO: Add network stack documentation. :) On 2015/01/23 15:04:18, rdsmith wrote: > Cute, but I wince at it too; it feels like we shouldn't commit a file that's > purely a Todo. Can you write up a bit about what the network stack is, what the > important subdirectories are, and what the plan for documentation is, to go in > here? That's a good bit of work itself; I'd rather either put in a placeholder here or just not have the file at all. https://codereview.chromium.org/868843002/diff/20001/net/net.gypi File net/net.gypi (right): https://codereview.chromium.org/868843002/diff/20001/net/net.gypi#newcode1782 net/net.gypi:1782: 'net_docs_output_dir': '<(PRODUCT_DIR)/net/docs', On 2015/01/23 15:04:18, rdsmith wrote: > My instinct looking at this file is that it's not the right place for > net_docs_script or net_docs_output_dir. But I'm not sure what would be the > right place, and I'm not even sure I'm right about this not being the right > place :-J. <noideadog/> It should be here; the expectation is that in the future, a .gn file will pull in these variables and use them, so we don't have to duplicate the script and directory names across the two files. https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:6: On 2015/01/23 15:04:18, rdsmith wrote: > I'm finding myself struck by two things about this file: It's lack of > comments/documentation, and it's lack of object orientation. Willing to rework > it to remedy those lacks? > > I'm uncertain about the OO--I don't want to simply assume that OO is always > better. But the python scripts I've seen us use in Chrome are pretty > consistently OO with a bit of a procedural wrapper at the end for running the > script, and being consistent is good :-}. It does allow for composition in the > future (easier to turn the file into a module or add stuff without worrying > about namespace collisions). And OO does allow more natural places for > documentation (class doc and method doc). > > My thought is having an object per markdown file, and having all the work done > in methods on that object. But my concern is with the OO. I haven't split it out entirely into separate files because I want to be able to some work that's shared among files, like generating a directory tree linking to all docs, or something like that. I was originally imagining a DocProcessor object, but then I realized the interface I was inventing was basically "build a list, set a couple of parameters, and then do a thing", which is perfectly serviceable as a function. (I'm happy to discuss more specific solutions, though.) https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:7: On 2015/01/23 15:04:18, rdsmith wrote: > nit: extra blank line. Done. https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:31: TEMPLATE = '<html><head><title>%s</title></head><body>%s</body></html>' On 2015/01/23 15:04:18, rdsmith wrote: > Suggestion: Pull this out of the function and make it a multi-line string, just > to make it easier to read and modify separately. Done.
Responses to comments. https://codereview.chromium.org/868843002/diff/20001/net/README.md File net/README.md (right): https://codereview.chromium.org/868843002/diff/20001/net/README.md#newcode1 net/README.md:1: TODO: Add network stack documentation. :) On 2015/01/23 20:13:00, ttuttle wrote: > On 2015/01/23 15:04:18, rdsmith wrote: > > Cute, but I wince at it too; it feels like we shouldn't commit a file that's > > purely a Todo. Can you write up a bit about what the network stack is, what > the > > important subdirectories are, and what the plan for documentation is, to go in > > here? > > That's a good bit of work itself; I'd rather either put in a placeholder here or > just not have the file at all. So I have a rule of thumb not to put in supporting infrastructure until it's needed, meaning I generally commit infrastructure with the first use of it in the system. Now, like "tests in the same CL as code", that's mostly to make sure that I don't go infrastructure-happy without there being a clear and present need, and we clearly want to move in the MD direction. So I'm ok with just a placeholder. No file at all wouldn't be ok, though (as you'd be effectively committing things without a test case). (I could see an argument that you should have *some* markdown in this file, just to make sure that it's actually being processed properly. But I'm ok without.) In other words: Ok :-}. https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:6: On 2015/01/23 20:13:00, ttuttle wrote: > On 2015/01/23 15:04:18, rdsmith wrote: > > I'm finding myself struck by two things about this file: It's lack of > > comments/documentation, and it's lack of object orientation. Willing to > rework > > it to remedy those lacks? > > > > I'm uncertain about the OO--I don't want to simply assume that OO is always > > better. But the python scripts I've seen us use in Chrome are pretty > > consistently OO with a bit of a procedural wrapper at the end for running the > > script, and being consistent is good :-}. It does allow for composition in > the > > future (easier to turn the file into a module or add stuff without worrying > > about namespace collisions). And OO does allow more natural places for > > documentation (class doc and method doc). > > > > My thought is having an object per markdown file, and having all the work done > > in methods on that object. But my concern is with the OO. > > I haven't split it out entirely into separate files because I want to be able to > some work that's shared among files, like generating a directory tree linking to > all docs, or something like that. Hmmm. I wouldn't think that would argue against doing something in an OO fashion, you'd just need a traversal interface for the objects. > I was originally imagining a DocProcessor object, but then I realized the > interface I was inventing was basically "build a list, set a couple of > parameters, and then do a thing", which is perfectly serviceable as a function. No argument, I'm just inclined to think the namespace and encapsulation is better as an object. But I'm willing to leave this to someone who knows the Google python style guide, if you feel strongly about it. > > (I'm happy to discuss more specific solutions, though.) It looks like you ignored my request for some documentation in the file?
PTAL at new sys.path hack; I still need to add some documentation to the script. https://codereview.chromium.org/868843002/diff/20001/net/README.md File net/README.md (right): https://codereview.chromium.org/868843002/diff/20001/net/README.md#newcode1 net/README.md:1: TODO: Add network stack documentation. :) On 2015/01/26 17:29:34, rdsmith wrote: > On 2015/01/23 20:13:00, ttuttle wrote: > > On 2015/01/23 15:04:18, rdsmith wrote: > > > Cute, but I wince at it too; it feels like we shouldn't commit a file that's > > > purely a Todo. Can you write up a bit about what the network stack is, what > > the > > > important subdirectories are, and what the plan for documentation is, to go > in > > > here? > > > > That's a good bit of work itself; I'd rather either put in a placeholder here > or > > just not have the file at all. > > So I have a rule of thumb not to put in supporting infrastructure until it's > needed, meaning I generally commit infrastructure with the first use of it in > the system. Now, like "tests in the same CL as code", that's mostly to make > sure that I don't go infrastructure-happy without there being a clear and > present need, and we clearly want to move in the MD direction. So I'm ok with > just a placeholder. No file at all wouldn't be ok, though (as you'd be > effectively committing things without a test case). (I could see an argument > that you should have *some* markdown in this file, just to make sure that it's > actually being processed properly. But I'm ok without.) > > In other words: Ok :-}. Acknowledged. https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/20001/net/tools/net_docs/net_d... net/tools/net_docs/net_docs.py:6: On 2015/01/26 17:29:34, rdsmith wrote: > On 2015/01/23 20:13:00, ttuttle wrote: > > On 2015/01/23 15:04:18, rdsmith wrote: > > > I'm finding myself struck by two things about this file: It's lack of > > > comments/documentation, and it's lack of object orientation. Willing to > > rework > > > it to remedy those lacks? > > > > > > I'm uncertain about the OO--I don't want to simply assume that OO is always > > > better. But the python scripts I've seen us use in Chrome are pretty > > > consistently OO with a bit of a procedural wrapper at the end for running > the > > > script, and being consistent is good :-}. It does allow for composition in > > the > > > future (easier to turn the file into a module or add stuff without worrying > > > about namespace collisions). And OO does allow more natural places for > > > documentation (class doc and method doc). > > > > > > My thought is having an object per markdown file, and having all the work > done > > > in methods on that object. But my concern is with the OO. > > > > I haven't split it out entirely into separate files because I want to be able > to > > some work that's shared among files, like generating a directory tree linking > to > > all docs, or something like that. > > Hmmm. I wouldn't think that would argue against doing something in an OO > fashion, you'd just need a traversal interface for the objects. > > > I was originally imagining a DocProcessor object, but then I realized the > > interface I was inventing was basically "build a list, set a couple of > > parameters, and then do a thing", which is perfectly serviceable as a > function. > > No argument, I'm just inclined to think the namespace and encapsulation is > better as an object. But I'm willing to leave this to someone who knows the > Google python style guide, if you feel strongly about it. Eh, let's sit down in person for a couple of minutes and see if we can work out a better interface. Right now I like how simple the not-object-oriented version is. > > > > (I'm happy to discuss more specific solutions, though.) > > It looks like you ignored my request for some documentation in the file? Nah, just didn't notice it. Will do.
PTAL, rdsmith. (Added comments to net_docs.py.)
Looks good except for the issues below (and I would like someone who knows the python style guide and how we do python in Chromium to look it over before landing). https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:58: # TODO(ttuttle): Add navigation? I'm probably being clueless, but I'm not sure what this means. What kind of navigation would you add? Markdown already allows for links. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:62: nit, suggestion: I don't think an extra blank line here adds anything (i.e. I don't think that the above functions and the below functions are different enough to make it make sense). https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:68: not specified, simply ensures the files exist and contain valid Markdown. Why is input_pathname required to produce output? I would think that if the script can find the input file and has a place to put the output, it would do so? https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:96: # TODO(ttuttle): Sanitize HTML? If possible, I'd like to resolve this TODO before landing. My rationale is that if it's a security issue, we need to sanitize the HTML before putting the functionality in the tree for the first time, and if it's not a security issue, we don't need the TODO. I think it's a security issue in the sense that a Chromium committer can commit code that'll be run by many many many people, so the code in the Chromium code base is sensitive. However, sanitizing the HTML won't really help that; such a hypothetical malcommitter would just edit url_request.cc instead. So my inclination is that you don't need to sanitize the HTML. But if you see a different threat model, please let me know.
PTAL, rdsmith. Any suggestions for a Python guru to take a finer pass at it? https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:58: # TODO(ttuttle): Add navigation? On 2015/02/16 14:17:37, rdsmith wrote: > I'm probably being clueless, but I'm not sure what this means. What kind of > navigation would you add? Markdown already allows for links. I meant like a directory tree of available documentation. If you don't think it's needed, I'll skip it. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:62: On 2015/02/16 14:17:36, rdsmith wrote: > nit, suggestion: I don't think an extra blank line here adds anything (i.e. I > don't think that the above functions and the below functions are different > enough to make it make sense). Oh, whoops, that's a typo. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:68: not specified, simply ensures the files exist and contain valid Markdown. On 2015/02/16 14:17:37, rdsmith wrote: > Why is input_pathname required to produce output? I would think that if the > script can find the input file and has a place to put the output, it would do > so? It needs the input pathname so it can figure out what portion of the file path is "path within the source tree" and what portion is just parent directories. That is, it can't go from src/net/sdch/README.md to out/Debug/net/docs/sdch/README.md.html without knowing to *remove src/net* before prepending out/Debug/net/docs. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:96: # TODO(ttuttle): Sanitize HTML? On 2015/02/16 14:17:37, rdsmith wrote: > If possible, I'd like to resolve this TODO before landing. My rationale is that > if it's a security issue, we need to sanitize the HTML before putting the > functionality in the tree for the first time, and if it's not a security issue, > we don't need the TODO. > > I think it's a security issue in the sense that a Chromium committer can commit > code that'll be run by many many many people, so the code in the Chromium code > base is sensitive. However, sanitizing the HTML won't really help that; such a > hypothetical malcommitter would just edit url_request.cc instead. So my > inclination is that you don't need to sanitize the HTML. But if you see a > different threat model, please let me know. Yeah, fair, I guess I won't.
LGTM with below nits. Not a request or criticism, just a thought for you to consider: I find it useful as a reviewer when someone syncs to a different revision in ToT that that's a separate PS in the review. That way if I'm scanning inter-patchset diffs, I can ignore the sync to the new revision and just focus on actual changes the person made in the code. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:58: # TODO(ttuttle): Add navigation? On 2015/02/20 21:03:55, ttuttle wrote: > On 2015/02/16 14:17:37, rdsmith wrote: > > I'm probably being clueless, but I'm not sure what this means. What kind of > > navigation would you add? Markdown already allows for links. > > I meant like a directory tree of available documentation. If you don't think > it's needed, I'll skip it. No, sounds like a reasonable idea, and the todo is a reasonable place for it. I'd just like more detail in the TODO() in case it's someone besides you who gets the bit between their teeth and does it. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:68: not specified, simply ensures the files exist and contain valid Markdown. On 2015/02/20 21:03:55, ttuttle wrote: > On 2015/02/16 14:17:37, rdsmith wrote: > > Why is input_pathname required to produce output? I would think that if the > > script can find the input file and has a place to put the output, it would do > > so? > > It needs the input pathname so it can figure out what portion of the file path > is "path within the source tree" and what portion is just parent directories. > > That is, it can't go from src/net/sdch/README.md to > out/Debug/net/docs/sdch/README.md.html without knowing to *remove src/net* > before prepending out/Debug/net/docs. Ah, that makes sense. Add a line indicating that that transformation is applied for context?
rdsmith@chromium.org changed reviewers: + phajdan.jr@chromium.org
Whoops, completely forgot to respond to your question about reviewers. Pawel, are you a reasonable person to take a look at this?
On 2015/02/23 18:48:33, rdsmith wrote: > Whoops, completely forgot to respond to your question about reviewers. Pawel, > are you a reasonable person to take a look at this? (Context: I want someone who's familiar with the python coding standard to sign off on it as well.)
LGTM w/nits https://codereview.chromium.org/868843002/diff/120001/net/tools/net_docs/net_... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/120001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:2: nit: Remove this empty line. https://codereview.chromium.org/868843002/diff/120001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:3: # Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015
Thanks, Randy and Paweł. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:58: # TODO(ttuttle): Add navigation? On 2015/02/23 14:42:26, rdsmith wrote: > On 2015/02/20 21:03:55, ttuttle wrote: > > On 2015/02/16 14:17:37, rdsmith wrote: > > > I'm probably being clueless, but I'm not sure what this means. What kind of > > > navigation would you add? Markdown already allows for links. > > > > I meant like a directory tree of available documentation. If you don't think > > it's needed, I'll skip it. > > No, sounds like a reasonable idea, and the todo is a reasonable place for it. > I'd just like more detail in the TODO() in case it's someone besides you who > gets the bit between their teeth and does it. Done. https://codereview.chromium.org/868843002/diff/100001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:68: not specified, simply ensures the files exist and contain valid Markdown. On 2015/02/23 14:42:25, rdsmith wrote: > On 2015/02/20 21:03:55, ttuttle wrote: > > On 2015/02/16 14:17:37, rdsmith wrote: > > > Why is input_pathname required to produce output? I would think that if the > > > script can find the input file and has a place to put the output, it would > do > > > so? > > > > It needs the input pathname so it can figure out what portion of the file path > > is "path within the source tree" and what portion is just parent directories. > > > > That is, it can't go from src/net/sdch/README.md to > > out/Debug/net/docs/sdch/README.md.html without knowing to *remove src/net* > > before prepending out/Debug/net/docs. > > Ah, that makes sense. Add a line indicating that that transformation is applied > for context? Done. https://codereview.chromium.org/868843002/diff/120001/net/tools/net_docs/net_... File net/tools/net_docs/net_docs.py (right): https://codereview.chromium.org/868843002/diff/120001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:2: On 2015/02/24 13:23:47, Paweł Hajdan Jr. wrote: > nit: Remove this empty line. Done. https://codereview.chromium.org/868843002/diff/120001/net/tools/net_docs/net_... net/tools/net_docs/net_docs.py:3: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/02/24 13:23:46, Paweł Hajdan Jr. wrote: > nit: 2015 Done.
The CQ bit was checked by ttuttle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/868843002/#ps140001 (title: "Make requested changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/868843002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a898dd47f3cadba45edc5fa57c9772545bf8b8f2 Cr-Commit-Position: refs/heads/master@{#317825} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
