Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

convertDoxygen: Add docstrings to more python objects #2526

Merged
merged 3 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.p4*
.DS_Store
.AppleDouble

*.pyc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're adding this - don't suppose you could add __pycache__ to the list as well?
Had that queued up in another PR, but if you just add that to your MR, would avoid a merge conflict...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chadrik, is the .gitignore change critical for the main goal of this PR? For tracking changes for USD releases, it helps if all the changes for a PR are related.

If the .gitignore change isn't specifically needed for this PR, would it be possible to move the .gitignore change (maybe combined with @pmolodo's change) to a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed!

12 changes: 9 additions & 3 deletions docs/python/convertDoxygen.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,13 @@
# Loop through module list and create a Writer for each module to
# load and generate the doc strings for the specific module
for moduleName in moduleList:
if not moduleName:
continue
writer = Writer(packageName, moduleName)
# Parser.traverse builds the docElement tree for all the
# doxygen XML files, so we only need to call it once if we're
# processing multiple modules
if (docList == None):
if (docList is None):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using is instead of == is considered idiomatic because None is a singleton.

docList = parser.traverse(writer)
Debug("Processed %d DocElements from doxygen XML" % len(docList))
Debug("Processing module %s" % moduleName)
Expand All @@ -120,8 +122,12 @@
module_output_file = os.path.join(module_output_dir, "__DOC.py")
writer.generate(module_output_file, docList)
else:
moduleName = modules
# Processing a single module. Writer's constructor will sanity
# check module and verify it can be loaded
writer = Writer()
if not output_file.endswith(".py"):
module_output_dir = os.path.join(output_file, moduleName)
output_file = os.path.join(module_output_dir, "__DOC.py")
writer = Writer(packageName, moduleName)
docList = parser.traverse(writer)
writer.generate(output_file, docList)
writer.generate(output_file, docList)
23 changes: 19 additions & 4 deletions docs/python/doxygenlib/cdDocElement.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,14 @@
#
# The Parser class is responsible for building up a list of these objects.
#
from collections import namedtuple

from .cdUtils import Warn


Param = namedtuple("Param", "type name default")


class DocElement:
"""
Describe the documentation for a single class, method, function, etc.
Expand All @@ -45,6 +50,9 @@ class DocElement:
same function, i.e., in C++ parlance, the overloaded methods.
"""

__slots__ = ("name", "kind", "prot", "doc", "location", "children", "const", "virt", "explicit",
"static", "inline", "returnType", "argsString", "definition", "params")

def __init__(self, name, kind, prot, doc, location):
self.name = name # the name of this class/method
self.kind = kind # e.g., function, class, etc.
Expand All @@ -60,7 +68,10 @@ def __init__(self, name, kind, prot, doc, location):
self.returnType = None # return type of a method/function
self.argsString = None # arguments for this method/func
self.definition = None # full C++ definition for method
self.params = None # name and type of each parameter
self.params = None # type, name, and default of each parameter

def __repr__(self):
return "%s(%r, %r, %r, ...)" % (self.__class__.__name__, self.name, self.kind, self.location)

def isFunction(self):
"""Is this doc element a function?"""
Expand All @@ -86,6 +97,10 @@ def isRoot(self):
"""Is this doc element the root of the doxygen XML tree?"""
return self.kind == 'root'

def isStatic(self):
"""Is this doc element static?"""
return self.static is not None and self.static == 'yes'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added isStatic and __repr__ methods for convenience

def addChildren(self, children):
"""Adds the list of nodes as children of this node."""
for child in children:
Expand All @@ -103,7 +118,7 @@ def replaceInnerClass(self, innerClassName, obj):
del(self.children[childName])
self.__addChild(obj)
return
Warn('could not find innerclass %s in %s' % (innerClassName,self.name))
Warn('%r: could not find innerclass %s in %s' % (self, innerClassName,self.name))

def __addChild(self, child):
if child.name in self.children:
Expand All @@ -120,8 +135,8 @@ def __addChild(self, child):
# so just ignore it.
pass
else:
Warn('overload mismatch: expected functions, got %s and %s' % \
(self.children[child.name][0].kind, child.kind))
Warn('%r: overload mismatch: expected functions, got %s and %s' % \
(self, self.children[child.name][0].kind, child.kind))
else:
self.children[child.name] = [child]

Expand Down
11 changes: 8 additions & 3 deletions docs/python/doxygenlib/cdParser.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,18 @@ class XMLNode:
"""
Rrepresent a single node in the XML tree.
"""
__slots__ = ("parent", "name", "attrs", "text", "childNodes")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added slots for performance: we make a lot of these classes.

def __init__(self, parent, name, attrs, text):
self.parent = parent
self.name = name
self.attrs = attrs
self.text = text
self.childNodes = []

def __repr__(self) -> str:
return "XMLNode(%s, %s, ...)" % (self.name, self.attrs.items())

def addChildNode(self, node):
"""Append the specifed node to the children of this node."""
self.childNodes.append(node)
Expand Down Expand Up @@ -221,8 +226,7 @@ def parseDoxygenIndexFile(self, doxygen_index_file):
continue
if (kind == "dir"):
continue
if (kind == "file"):
continue
Comment on lines -224 to -225
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think this change is the heart of this PR - unfortunately, when we tested it, it had some negative side effects.
Specifically, many of our classes lost their docstrings after running convertDoxygen.py with this change.

One example was pxr.Usd.ZipFileWriter - in Usd/__DOC.py, we now get:

   result["ZipFileWriter"].__doc__ = """"""

instead of:

   result["ZipFileWriter"].__doc__ = """
Class for writing a zip file. This class is primarily intended to
support the .usdz file format. It is not a general-purpose zip writer,
as it does not implement the full zip file specification. However, all
files written by this class should be valid zip files and readable by
external zip modules and utilities.

"""

We're using Doxygen 1.8.18... which is old, but also newer than the 1.8.14 listed in VERSIONS.md.

@chadrik, can you confirm whether or not you're seeing the same, and what version of Doxygen you're using?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @pmolodo , you made us realize we neglected to update VERSIONS.md for the addition of doxygen-awesome, which requires 1.9.x (we are using 1.9.6 internally). We will be pushing out updates to VERSIONS.md shortly, but can you retry your experiments with a 1.9.x version?

Copy link

@dsyu-pixar dsyu-pixar Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a test with only removing these lines and comparing build output (using doxygen 1.8.x). The new __DOC.py appears to have duplicate (empty) values for some things, like ZipFileWriter as @pmolodo notes, which end up overwriting the earlier defined values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed.

# we need to keep kind == "file" because this holds info on functions
refid = compound_element.get('refid')
# Individual entity XML generated XML files are <refid>.xml
entity_file_name = refid + ".xml"
Expand Down Expand Up @@ -357,7 +361,8 @@ def __getAllParams(self, node):
if child.name == 'param':
pname = child.getText('declname')
ptype = child.getText('type')
params.append((ptype, pname))
pdefault = child.getText('defval') or None
params.append(Param(ptype, pname, pdefault))
return params

def __createDocElement(self, node):
Expand Down
47 changes: 15 additions & 32 deletions docs/python/doxygenlib/cdWriterDocstring.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,29 +52,6 @@ class Writer:
# we can combine property docstrings for getters/setters.
propertyTable = {}

# Default constructor that assumes we're processing a single module
def __init__(self):
#
Copy link
Contributor Author

@chadrik chadrik Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python doesn't support overloads like C++: you can't simply define a function twice. This __init__ was being replaced by the one below it, so calling Writer() without args never actually worked. I've fixed the use of this class in convertDoxygen

# Parse the extra arguments required by this plugin
#
project = GetArgValue(['--package', '-p'])
package = GetArgValue(['--module', '-m'])

if not project:
Error("Required option --package not specified")
if not package:
Error("Required option --module not specified")

# Import the python module that these docs pertain to
if not Import("from " +project+ " import " +package+ " as " +package):
Error("Could not import %s" % (package))

self.module = eval(package)
self.prefix = self.module.__name__.split('.')[-1]
self.seenPaths = {}

# Constructor that takes package and module name, used when processing
# a list of modules
def __init__(self, packageName, moduleName):

# Import the python module
Expand Down Expand Up @@ -295,11 +272,14 @@ def generate(self, output_file, docElements):
if len(lines) == 1:
lines.append(" pass")

outputDir = os.path.split(output_file)[0]
if not os.path.exists(outputDir):
os.makedirs(outputDir)

# output the lines to disk...
try:
logfile = open(output_file, 'w')
logfile.write('\n'.join(lines))
logfile.close()
with open(output_file, 'w') as logfile:
logfile.write('\n'.join(lines))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use context manager to ensure that the file is closed in case of an exception (also, more idiomatic)

except:
Error("Could not write to file: %s" % output_file)

Expand Down Expand Up @@ -502,11 +482,14 @@ def __getSignatureString(self, pyname, pyobj, doxy):
if doxy.isFunction():
cnt = 1;
pnames = []
for ptype, pname in doxy.params:
for ptype, pname, pdefault in doxy.params:
if len(pname):
pnames.append(pname)
arg = pname
else:
pnames.append('arg%s' % cnt)
arg = 'arg%s' % cnt
if pdefault is not None:
arg += '=...'
pnames.append(arg)
cnt += 1
sig = '('+', '.join(pnames)+')'
retType = self.__convertTypeName(doxy.returnType)
Expand All @@ -520,7 +503,7 @@ def __getSignatureDescription(self, pyname, pyobj, doxy):
if doxy.isFunction():
cnt = 0
lines = []
for ptype, pname in doxy.params:
for ptype, pname, pdefault in doxy.params:
cnt += 1
if not len(pname):
pname = 'arg%s' % cnt
Expand Down Expand Up @@ -593,11 +576,11 @@ def __getOutputFormat(self, pypath, pyobj, overloads):

if len(overloads) == 1:
lines += self.__getFullDoc(pyname, pyobj, overloads[0])
if overloads[0].static == 'yes':
if overloads[0].isStatic():
docString = LABEL_STATIC # set the return type to static
else:
for doxy in overloads:
if doxy.static == 'yes':
if doxy.isStatic():
docString = LABEL_STATIC # set the return type to static

desc = self.__getFullDoc(pyname, pyobj, doxy)
Expand Down