freetype-devel
[Top][All Lists]
Advanced

[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(…)
```


reply via email to

[Prev in Thread] Current Thread [Next in Thread]