> Hi Nikhil, > I had a superficial look: > > * I still recommend an autoformatter because it eliminates a certain class > of error: the formatting nit before merging a PR. Otherwise, you end up > micromanaging e.g. spaced () ( > https://github.com/nikramakrishnan/freetype-docwriter/blob/master/content.py#L113) > and unspaced () ( > https://github.com/nikramakrishnan/freetype-docwriter/blob/master/content.py#L143 > ) >
I would really like to use a formatting tool like yapf, but the only problem is that it doesn't have a configuration/option for the foo( bar ) format (space in brackets). I checked options from https://github.com/google/yapf/blob/master/README.rst#knobs. Am I missing something? > * I also suggest you keep to testing 2.7 and 3.6 or 3.7. Is there a > requirement for testing 3.4 and 3.5? > 3.7 is not supported on Travis CI yet. I'm testing 2.7, 3.5 and 3.6 now. > * In e.g. formatter.py, you duplicate the module docstring in the comment > above. This is redundant, I recommend writing everything important into the > docstring, as that is accessible in Python. > This is fixed. Module descriptions are in docstrings now. * Any reason for the "# eof“ comments at the bottom? > This was already present in docmaker, and other source files in the library also mark eofs, so I figured this was somewhat important. > * I suggest you fix the `import *` statements and instead be explicit. > This is fixed, thanks. > * You might want to use the argparse module instead of getopt, that should > simplify docwriter.py significantly. > https://docs.python.org/3/howto/argparse.html. Writing your own usage() > is inelegant. > I was actually thinking about replacing getopt because of the increasing work required to maintain the usage() function. Working on this right now. It is indeed much simpler to work with argparse. As a very general aside, I suggest you avoid doing anything other than > assigning arguments in __init__ and instead use @classmethod constructors > for initializing everything you need for a class. See e.g. > http://as.ynchrono.us/2014/12/asynchronous-object-initialization.html. > Using the new @dataclass or the attrs module will nudge you into this > direction by writing __init__ for you. I’m not saying you should do this > right now, as this requires re-designing most classes. But take it as a > suggestion for future work. The general pattern would be: [...] > I'll have a look at this, thanks! -- Nikhil
_______________________________________________ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
