[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
From: |
Nikolaus Waxweiler |
Subject: |
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting |
Date: |
Thu, 12 Jul 2018 20:08:26 +0200 |
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 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?
* 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.
* Any reason for the "# eof“ comments at the bottom?
* I suggest you fix the `import *` statements and instead be explicit.
* 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.
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:
```
class Formatter(object):
def __init__(self, processor, identifiers, chapters, sections, block_index):
self.processor = processor
self.identifiers = identifiers
self.chapters = chapters
self.sections = sections
self.block_index = block_index
@classmethod
def with_processor(processor):
# construct all that's needed
return cls(processor, identifiers, chapters, sections, block_index)
# …
# Instantiation:
formatter = Formatter.with_processor(…)
```
- [ft-devel] Updates to Docwriter: Logging, testing, linting, Nikhil Ramakrishnan, 2018/07/12
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting,
Nikolaus Waxweiler <=
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Werner LEMBERG, 2018/07/12
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Nikhil Ramakrishnan, 2018/07/14
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Nikolaus Waxweiler, 2018/07/14
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Nikhil Ramakrishnan, 2018/07/14
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Nikolaus Waxweiler, 2018/07/14
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Werner LEMBERG, 2018/07/14
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Werner LEMBERG, 2018/07/14
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Nikolaus Waxweiler, 2018/07/14
- Re: [ft-devel] Updates to Docwriter: Logging, testing, linting, Werner LEMBERG, 2018/07/14