Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1574883002:
add doxypypy and py_filter so this will turn google style (Closed)
|
Created:
4 years, 11 months ago by rkotlerimgtec Modified:
4 years, 10 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Descriptionadd doxypypy support to Subzero doxygen
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=7910a26d8d936fe8a4f834bcdf1be037eff6228b
Patch Set 1 #
Total comments: 8
Patch Set 2 : changes suggested by stichnot #Patch Set 3 : changes suggested by stichnode #Patch Set 4 : changes suggested by stichnot #Patch Set 5 : changes suggested by stichnot #
Total comments: 15
Patch Set 6 : changes suggested by stichnot #Patch Set 7 : left makefile in test state. #
Messages
Total messages: 25 (4 generated)
Description was changed from ========== add doxypypy and py_filter so this will turn google style python docstrings into the right thing for doxygen BUG= ========== to ========== add doxypypy and py_filter so this will turn google style python docstrings into the right thing for doxygen BUG= ==========
rkotlerimgtec@gmail.com changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org
This really improves the doxygen for python files. Basically it's a doxygen filter that will turn the doc strings in the python code into the proper doxygen syntax. Even for simple docstrings this works better because previously those show up as function detailed documentation but not in the brief documentation. it would make more sense to add doxypypy to the depot_tools than to check it in here but that is not something i'm able to do. I just created a py_filter and followed the instructions in https://github.com/Feneric/doxypypy . This tool understands all the google python docstring attributes.
1. Please follow the git standard of a concise one-line <80-col summary to start the description / commit message. 2. It does look like this generates marginally better summaries for the python functions. Specifically, doc snippets in e.g. file:///home/stichnot/nacl/native_client/toolchain_build/src/subzero/build/docs/html/szbuild_8py.html where there were none before. (Not sure if there are more features I should be looking for.) However, it gives "More..." links that just go to the same page. 3. I will have to look into whether this GPL license can be dropped into a subdirectory without affecting the licensing of the entire project. https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore File doxypypy/.gitignore (right): https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore#newcode39 doxypypy/.gitignore:39: Trailing whitespace makes git sad. https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst File doxypypy/README.rst (right): https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode184 doxypypy/README.rst:184: .. _PEP 257: http://www.python.org/dev/peps/pep-0257/ use https https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode185 doxypypy/README.rst:185: .. _Google Python Style Guide: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... This link is broken (plus it's not https). https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode186 doxypypy/README.rst:186: Trailing whitespace.
I can upload an example, but it does quite a bit more for python than the vanilla doxygen python support. The basic doxygen python support seems to only look at docstrings for functions and the rest you need to with normal doxygen style markup; and even in that case it does not have brief description which is really useful. The main conflict is google style python documentation vs normal doxygen style. If you want to use docstrings per Google style, this tool helps a lot because it translates all of them into the doxygen form and then you get the nice doxygen output. On Mon, Jan 11, 2016 at 6:44 AM, <stichnot@chromium.org> wrote: > 1. Please follow the git standard of a concise one-line <80-col summary to > start > the description / commit message. > > 2. It does look like this generates marginally better summaries for the > python > functions. Specifically, doc snippets in e.g. > > file:///home/stichnot/nacl/native_client/toolchain_build/src/subzero/build/docs/html/szbuild_8py.html > where there were none before. (Not sure if there are more features I > should be > looking for.) However, it gives "More..." links that just go to the same > page. > > 3. I will have to look into whether this GPL license can be dropped into a > subdirectory without affecting the licensing of the entire project. > > > https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore > File doxypypy/.gitignore (right): > > > https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore#newcode39 > doxypypy/.gitignore:39: > Trailing whitespace makes git sad. > > https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst > File doxypypy/README.rst (right): > > > https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode184 > doxypypy/README.rst:184: .. _PEP 257: > http://www.python.org/dev/peps/pep-0257/ > use https > > > https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode185 > doxypypy/README.rst:185: .. _Google Python Style Guide: > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... > This link is broken (plus it's not https). > > > https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode186 > doxypypy/README.rst:186: > Trailing whitespace. > > https://codereview.chromium.org/1574883002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
There are lots of samples in doxypypy/doxypypy/test <https://codereview.chromium.org/1574883002/patch/1/10011> including one which tests a lot of the special Google interpretation of docstrings. https://codereview.chromium.org/1574883002/patch/1/10020 You can look at what the script does to them. For exampe for the googletest sample, it turns it into: https://codereview.chromium.org/1574883002/patch/1/10016 On Mon, Jan 11, 2016 at 10:40 AM, reed kotler <rkotlerimgtec@gmail.com> wrote: > I can upload an example, but it does quite a bit more for python than the > vanilla doxygen python support. > The basic doxygen python support seems to only look at docstrings for > functions and the rest you need to with > normal doxygen style markup; and even in that case it does not have brief > description which is really useful. > > The main conflict is google style python documentation vs normal doxygen > style. > > If you want to use docstrings per Google style, this tool helps a lot > because it translates > all of them into the doxygen form and then you get the nice doxygen output. > > > On Mon, Jan 11, 2016 at 6:44 AM, <stichnot@chromium.org> wrote: > >> 1. Please follow the git standard of a concise one-line <80-col summary >> to start >> the description / commit message. >> >> 2. It does look like this generates marginally better summaries for the >> python >> functions. Specifically, doc snippets in e.g. >> >> file:///home/stichnot/nacl/native_client/toolchain_build/src/subzero/build/docs/html/szbuild_8py.html >> where there were none before. (Not sure if there are more features I >> should be >> looking for.) However, it gives "More..." links that just go to the same >> page. >> >> 3. I will have to look into whether this GPL license can be dropped into a >> subdirectory without affecting the licensing of the entire project. >> >> >> https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore >> File doxypypy/.gitignore (right): >> >> >> https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore#newcode39 >> doxypypy/.gitignore:39: >> Trailing whitespace makes git sad. >> >> https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst >> File doxypypy/README.rst (right): >> >> >> https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode184 >> doxypypy/README.rst:184: .. _PEP 257: >> http://www.python.org/dev/peps/pep-0257/ >> use https >> >> >> https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode185 >> doxypypy/README.rst:185: .. _Google Python Style Guide: >> >> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... >> This link is broken (plus it's not https). >> >> >> https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode186 >> doxypypy/README.rst:186: >> Trailing whitespace. >> >> https://codereview.chromium.org/1574883002/ >> > > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Description was changed from ========== add doxypypy and py_filter so this will turn google style python docstrings into the right thing for doxygen BUG= ========== to ========== add doxypypy support to Subzero doxygen BUG= ==========
If there is an issue checking in doxpypy, it's always possible to just add it to thirdparty or depot_tools https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore File doxypypy/.gitignore (right): https://codereview.chromium.org/1574883002/diff/1/doxypypy/.gitignore#newcode39 doxypypy/.gitignore:39: On 2016/01/11 14:44:46, stichnot wrote: > Trailing whitespace makes git sad. Done. https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst File doxypypy/README.rst (right): https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode184 doxypypy/README.rst:184: .. _PEP 257: http://www.python.org/dev/peps/pep-0257/ On 2016/01/11 14:44:46, stichnot wrote: > use https Done. https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode185 doxypypy/README.rst:185: .. _Google Python Style Guide: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen... On 2016/01/11 14:44:46, stichnot wrote: > This link is broken (plus it's not https). Done. https://codereview.chromium.org/1574883002/diff/1/doxypypy/README.rst#newcode186 doxypypy/README.rst:186: On 2016/01/11 14:44:46, stichnot wrote: > Trailing whitespace. Done.
Sorry for taking so long on this. It seems that this would be the first real example of third_party code being included in Subzero, so some new structure and guidelines should be followed. (The Doxyfile config file may be close, but probably not.) https://www.chromium.org/developers/adding-3rd-party-libraries documents the Chromium process for dealing with third-party code. I would structure this if possible as docs/third_party/doxypypy . Slightly different from the recommendation of putting it under src/third_party/ , or in a top-level third_party/ directory. My reasoning (and IANAL of course), is that this provides a greater separation between our mostly LLVM licensed source code and this new GPL code. Never mind the OWNERS file. Never mind running license check scripts. Never mind the DEPS file. There is another internal document on open source components in general, which has some good advice. The initial commit (i.e. this CL) should be the original, unmodified code from github, and then have a follow-on commit (new CL) that applies our local changes. This makes it much easier to document its provenance.
So you are saying that you will do the OWNERS file, DEPS and running license check scripts? On Wed, Jan 20, 2016 at 10:42 AM, <stichnot@chromium.org> wrote: > Sorry for taking so long on this. > > It seems that this would be the first real example of third_party code > being > included in Subzero, so some new structure and guidelines should be > followed. > (The Doxyfile config file may be close, but probably not.) > > https://www.chromium.org/developers/adding-3rd-party-libraries documents > the > Chromium process for dealing with third-party code. > > I would structure this if possible as docs/third_party/doxypypy . Slightly > different from the recommendation of putting it under src/third_party/ , > or in a > top-level third_party/ directory. My reasoning (and IANAL of course), is > that > this provides a greater separation between our mostly LLVM licensed source > code > and this new GPL code. > > Never mind the OWNERS file. Never mind running license check scripts. > Never > mind the DEPS file. > > There is another internal document on open source components in general, > which > has some good advice. The initial commit (i.e. this CL) should be the > original, > unmodified code from github, and then have a follow-on commit (new CL) that > applies our local changes. This makes it much easier to document its > provenance. > > https://codereview.chromium.org/1574883002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
On 2016/01/20 20:43:05, rkotlerimgtec wrote: > So you are saying that you will do the OWNERS file, DEPS and running > license check scripts? Ignore OWNERS and DEPS entirely, we won't do that in this paltry code base. I'll handle the license check scripts as a one-off since I have some Chrome build lying around.
I just realized that we can install this using easy_install or pip. That is another option. easy_install doxypypy pip install doxypypy Thoughts? On Wed, Jan 20, 2016 at 5:43 PM, <stichnot@chromium.org> wrote: > On 2016/01/20 20:43:05, rkotlerimgtec wrote: > >> So you are saying that you will do the OWNERS file, DEPS and running >> license check scripts? >> > > Ignore OWNERS and DEPS entirely, we won't do that in this paltry code base. > > I'll handle the license check scripts as a one-off since I have some Chrome > build lying around. > > https://codereview.chromium.org/1574883002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
I think there is something wrong with this patch where I moved doxypyy. I forgot that I had installed doxypypy. Upon uninstallying the make docs for python is not working properly. I wil investigate right now. On Thu, Jan 21, 2016 at 12:58 PM, <rkotlerimgtec@gmail.com> wrote: > https://codereview.chromium.org/1574883002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
Anything more you need me to do here? On Thu, Jan 21, 2016 at 2:49 PM, <rkotlerimgtec@gmail.com> wrote: > https://codereview.chromium.org/1574883002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
On 2016/01/25 20:44:04, rkotlerimgtec wrote: > Anything more you need me to do here? With this approach, I think it is missing the README.chromium file, and in particular I think it should document the git hash of the specific version being cloned here. That said, I looked into your earlier suggestion of "sudo pip install doxypypy". That seems vastly simpler! With that approach, I would put a test for doxypypy in the docs/Makefile.standalone - if it's not installed, print a helpful message and hard-fail, something like: mkdir -p ../build @python -m doxypypy.doxypypy /dev/null || \ (echo "Python doxypypy package needs to be installed, e.g." \ "'sudo pip install doxypypy'" ; exit 1) doxygen Doxyfile @echo See file://`pwd`/../build/docs/html/index.html
Ok. Yes, otherwise it probably would belong in depot tools or third party and require someone to be updating it. This way they can install it and we can check the version number too if we need to, Things will still work without this package installed, just not as well. We can just pass all the text through as is with a nop filter. On Tue, Jan 26, 2016 at 6:18 AM, <stichnot@chromium.org> wrote: > On 2016/01/25 20:44:04, rkotlerimgtec wrote: > >> Anything more you need me to do here? >> > > With this approach, I think it is missing the README.chromium file, and in > particular I think it should document the git hash of the specific version > being > cloned here. > > That said, I looked into your earlier suggestion of "sudo pip install > doxypypy". > That seems vastly simpler! With that approach, I would put a test for > doxypypy > in the docs/Makefile.standalone - if it's not installed, print a helpful > message > and hard-fail, something like: > > mkdir -p ../build > @python -m doxypypy.doxypypy /dev/null || \ > (echo "Python doxypypy package needs to be installed, e.g." \ > "'sudo pip install doxypypy'" ; exit 1) > doxygen Doxyfile > @echo See file://`pwd`/../build/docs/html/index.html > > > https://codereview.chromium.org/1574883002/ > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
I took out the doxypypy scripts and put it on the user to install these. If someone does a make of the docs without that, they will get a warning but it will still run doxygen, but just using the builtin doxygen python documentation support. Since python is a small part of the source base, I did not want to make it a fatal error if for some reason the user had trouble installing this doxypypy script.
https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalone File docs/Makefile.standalone (right): https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:6: all:: A normal "make docs" run produces pages and pages of output. No one will notice this warning output unless it is printed at the very end. https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:7: $(info "To get best Doxgen output for Python code") Doxygen https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:7: $(info "To get best Doxgen output for Python code") You should probably remove the double-quotes in the $(info) lines. They actually print out in the output. https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:8: $(info "Python doxypypy package should to be installed, e.g.") should be https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:12: $(info "see https://github.com/Feneric/doxypypy ") maybe drop the last space before the closing quote https://codereview.chromium.org/1574883002/diff/80001/docs/py_filter File docs/py_filter (right): https://codereview.chromium.org/1574883002/diff/80001/docs/py_filter#newcode4 docs/py_filter:4: ./fdoxypypy.py $1 Can you just do 'cat $1' instead of a special script? https://codereview.chromium.org/1574883002/diff/80001/docs/py_filter#newcode8 docs/py_filter:8: Remove trailing newlines
https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalone File docs/Makefile.standalone (right): https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:6: all:: On 2016/01/27 02:12:30, stichnot wrote: > A normal "make docs" run produces pages and pages of output. No one will notice > this warning output unless it is printed at the very end. Done. https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:7: $(info "To get best Doxgen output for Python code") On 2016/01/27 02:12:30, stichnot wrote: > Doxygen Done. https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:7: $(info "To get best Doxgen output for Python code") On 2016/01/27 02:12:30, stichnot wrote: > Doxygen Done. https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:7: $(info "To get best Doxgen output for Python code") On 2016/01/27 02:12:30, stichnot wrote: > You should probably remove the double-quotes in the $(info) lines. They > actually print out in the output. Done. https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:8: $(info "Python doxypypy package should to be installed, e.g.") On 2016/01/27 02:12:30, stichnot wrote: > should be Done. https://codereview.chromium.org/1574883002/diff/80001/docs/Makefile.standalon... docs/Makefile.standalone:12: $(info "see https://github.com/Feneric/doxypypy ") On 2016/01/27 02:12:30, stichnot wrote: > maybe drop the last space before the closing quote Done. https://codereview.chromium.org/1574883002/diff/80001/docs/py_filter File docs/py_filter (right): https://codereview.chromium.org/1574883002/diff/80001/docs/py_filter#newcode4 docs/py_filter:4: ./fdoxypypy.py $1 On 2016/01/27 02:12:30, stichnot wrote: > Can you just do 'cat $1' instead of a special script? Done. https://codereview.chromium.org/1574883002/diff/80001/docs/py_filter#newcode8 docs/py_filter:8: On 2016/01/27 02:12:30, stichnot wrote: > Remove trailing newlines Done.
nice, lgtm.
Description was changed from ========== add doxypypy support to Subzero doxygen BUG= ========== to ========== add doxypypy support to Subzero doxygen BUG= R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 7910a26d8d936fe8a4f834bcdf1be037eff6228b (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
