Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
It's funny Python often trades performance for elegance ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
>> > (Try to refactor code instead when you reach deeper levels) >> >> Which is not always a good idea; sometimes it is necessary to avoid >> subroutines for performance reasons, for example. > > You mean in Python? SCNR Yes. No idea how subroutines are officially called. And no idea why this is funny :-) Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> > > (Try to refactor code instead when you reach deeper levels) > > Which is not always a good idea; sometimes it is necessary to avoid > subroutines for performance reasons, for example. > You mean in Python? SCNR > ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
>> Thanks. I still prefer 2-space indentation; for deeper nesting >> levels it is easier to stay within the 78 (or 79) >> character-per-line limit... > > (Try to refactor code instead when you reach deeper levels) Which is not always a good idea; sometimes it is necessary to avoid subroutines for performance reasons, for example. >> Indeed. You shouldn't waste time on this issue since you already >> maintain well-formatted code. > > (The maintenance of which is an ongoing cost compared to the > automatism, I guess I'm the strange guy here, but I like formatting code manually. It helps me concentrate on the code and its flow. Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> > Thanks. I still prefer 2-space indentation; for deeper nesting levels > it is easier to stay within the 78 (or 79) character-per-line limit... > (Try to refactor code instead when you reach deeper levels) > I'll wait before applying a formatter till there is a consensus > > here :-) > > Indeed. You shouldn't waste time on this issue since you already > maintain well-formatted code. (The maintenance of which is an ongoing cost compared to the automatism, which is why I suggested a formatter in the first place. Also note that you can easily format everything later in one big commit and then ignore it when git blaming) ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> See the attached images. 'indent_4' is the current formatting, and > 'indent_2' is formatting done using 'yapf' with chromium style (2 > spaces) and column limit 78. Thanks. I still prefer 2-space indentation; for deeper nesting levels it is easier to stay within the 78 (or 79) character-per-line limit... > The auto formatters (both yapf and black), however, perform somewhat > undesirable changes at places. See 'regex_indent_old' and > 'regex_indent_new' as an example. Ouch. Looks like bugs in the formatters... >> In PEP 8, value 4 for indentation is like the parallel postulate: >> it is completely isolated from all other recommendations... > > I'm not sure what you mean by 'isolated' here, [...] I mean: If you change this value, it doesn't have any consequence to the remaining rules (the same is true for Euclid's parallel postulate). > I'll wait before applying a formatter till there is a consensus > here :-) Indeed. You shouldn't waste time on this issue since you already maintain well-formatted code. Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> > The auto formatters (both yapf and black), however, perform somewhat > undesirable changes at places. See 'regex_indent_old' and > 'regex_indent_new' > as an example. > Try dedenting the string and make the closing """ match up. I'd say the formatting is okay, things that don't fit a line get thrown on the next. it would be better to think about which is more readable and maintainable > for > us, IMO. > I find the blocks easier to differentiate with 4 spaces and too crowded with 2 spaces. > ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> There is no harm in everything looking the same; on the contrary, > new people familiar with other codebases can contribute more easily > because stuff looks the same — and since it’s automated, no PR ever > gets round-tripped because of some formatting nitpick. This is a very simplistic approach that doesn't fit all situations IMHO. > In fact, I can’t think of a concrete reason why a codebase should be > allowed to look different from other codebases. So tell me why Python itself allows a consistent but arbitrary number of spaces as indentation? > The dictator in this case is really PEP 8, black is just its > henchman. This approach is quite pythonic. In PEP 8, value 4 for indentation is like the parallel postulate: it is completely isolated from all other recommendations... Given that PEP 8 recommends a line length of 79 characters, it would be easier to obey this limitation if indentation is small. Whatever :-) Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> I strongly disagree. Within a project everything must be consistent, > no question. However, I don't see any reason that *all* projects must > look the same, with a single person (or a small team) being the > formatting dictator. There is no harm in everything looking the same; on the contrary, new people familiar with other codebases can contribute more easily because stuff looks the same — and since it’s automated, no PR ever gets round-tripped because of some formatting nitpick. In fact, I can’t think of a concrete reason why a codebase should be allowed to look different from other codebases. The dictator in this case is really PEP 8, black is just its henchman. This approach is quite pythonic. ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> I love it. Configurability is the enemy of consistency. I strongly disagree. Within a project everything must be consistent, no question. However, I don't see any reason that *all* projects must look the same, with a single person (or a small team) being the formatting dictator. Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
I love it. Configurability is the enemy of consistency. I get bouts of "E" when I have to look at other people's formatting preferences. ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> Uh oh. Black enforces 4 spaces for indentation. Honestly spoken, this would be an immediate reason for me to not use this tool, were I a Python programmer. Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
Uh oh. Black enforces 4 spaces for indentation. Black is the formatter with the most momentum right now. Yapf is more configurable but according to the black author not as consistent, which is why he wrote black in the first place. Good luck ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> I've applied black to docwriter.py in the latest commit where I > implement cli using argparse. It looks quite readable, IMHO. Indeed! > https://github.com/nikramakrishnan/freetype-docwriter/blob/f6f9dd07d89fa72bb5f17d563011e0790f2324a1/docwriter.py Minor nit: s/Setup the logger./Set up the logger./ Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> [...] `foo( bar )` is discouraged in Python-land: > https://www.python.org/dev/peps/pep-0008/#pet-peeves. :-) > If it’s okay with Werner, I strongly suggest going with idiomatic > Python style instead of carrying over the C style. The formatter > Black will do everything you need for that. BTW, I would like to have an indentation level of two spaces instead of four – I see no reason to waste so much horizontal space, and four spaces don't really increase readability IMHO. Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
Hello Nikhil! > 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). No need to retain that. You are going to completely rewrite the tool, so I don't mind if you change such code to `foo(bar)' in case you prefer that. What I want, however, is a mainly vertical code flow and lines not longer than 78 characters if possible. Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> You could also run it via tox on Travis: > https://github.com/fonttools/ufoLib2/blob/master/tox.ini > > Tox seemed too much for this module, and Travis does the multi-version checks > anyway. but I'll consider adding it. The prime advantage of using tox is that you can just type `tox` on your local machine _and_ on Travis and the testing procedure will be the same (ufoLib2 uses it in a way that pre-commit is run on each invocation, too). It’s not a must, of course. ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> There likely isn’t one because `foo( bar )` is discouraged in Python-land: > https://www.python.org/dev/peps/pep-0008/#pet-peeves. If it’s okay with > Werner, I strongly suggest going with idiomatic Python style instead of > carrying over the C style. The formatter Black will do everything you need > for that. > I've applied black to docwriter.py in the latest commit where I implement cli using argparse. It looks quite readable, IMHO. Of course, I'll only go ahead with this formatting style (PEP8 with line length 80) if Werner agrees. Here's the 'black' formatted version: https://github.com/nikramakrishnan/freetype-docwriter/blob/f6f9dd07d89fa72bb5f17d563011e0790f2324a1/docwriter.py > This is fixed. Module descriptions are in docstrings now. > > As I commented on GitHub, I recommend using > https://pypi.org/project/docformatter/ for formatting docstrings. > Yes, I'll add this, thanks. > In short: automate all the drudgery you can automate and you will increase > maintainability. > > > Speaking of automation, did I mention pre-commit yet? > https://pre-commit.com/, `pip install pre-commit`. > > It automates running Git hooks on your project on every commit. You can > e.g. automatically run black on the source, remove spurious line-end > whitespace, check YAML files and much more. > > You insert a file like > https://github.com/fonttools/ufoLib2/blob/master/.pre-commit-config.yaml > in your top-level and run `pre-commit install`. After that, every commit is > run through the gauntlet before being allowed to pass. Follow the repo URLs > to see available hooks to run. I personally like pyupgrade. This looks great, I'll check it out as soon as I can. > You could also run it via tox on Travis: > https://github.com/fonttools/ufoLib2/blob/master/tox.ini Tox seemed too much for this module, and Travis does the multi-version checks anyway. but I'll consider adding it. -- Nikhil ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> 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? There likely isn’t one because `foo( bar )` is discouraged in Python-land: https://www.python.org/dev/peps/pep-0008/#pet-peeves. If it’s okay with Werner, I strongly suggest going with idiomatic Python style instead of carrying over the C style. The formatter Black will do everything you need for that. > This is fixed. Module descriptions are in docstrings now. As I commented on GitHub, I recommend using https://pypi.org/project/docformatter/ for formatting docstrings. In short: automate all the drudgery you can automate and you will increase maintainability. Speaking of automation, did I mention pre-commit yet? https://pre-commit.com/, `pip install pre-commit`. It automates running Git hooks on your project on every commit. You can e.g. automatically run black on the source, remove spurious line-end whitespace, check YAML files and much more. You insert a file like https://github.com/fonttools/ufoLib2/blob/master/.pre-commit-config.yaml in your top-level and run `pre-commit install`. After that, every commit is run through the gauntlet before being allowed to pass. Follow the repo URLs to see available hooks to run. I personally like pyupgrade. You could also run it via tox on Travis: https://github.com/fonttools/ufoLib2/blob/master/tox.ini ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> 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
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> I've been working on improving docwriter's maintainability. These > are the changes made in the past few days: [...] Thanks for working on that. It looks very clean! Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> > > 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. > > [...] > > Since I have no idea what you are talking about, I won't comment this > further :-) It's a different way of designing classes with some benefits as explained in the article. > > ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
> * Any reason for the "# eof“ comments at the bottom? I want that; I consider it good practice to mark the intended end of file. > 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. > [...] Since I have no idea what you are talking about, I won't comment this further :-) Werner ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
Re: [ft-devel] Updates to Docwriter: Logging, testing, linting
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(…) ``` ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
[ft-devel] Updates to Docwriter: Logging, testing, linting
Hi all, I've been working on improving docwriter's maintainability. These are the changes made in the past few days: * Docwriter now uses the standard logging library of python, instead of printing directly to stderr. This gives greater control over messages and their formatting. * Docwriter can now be run with the `--quiet` or `--verbose` options (thanks to logging levels), and this is very helpful for debugging. * I have added Travis CI testing with various unit tests and an integration test. With this, every commit is tested with multiple versions (both 2 and 3) of Python. Test results are always available at https://github.com/nikramakrishnan/freetype-docwriter (in the README part below) and https://travis-ci.com/nikramakrishnan/freetype-docwriter. * I have also added Landscape's code 'health' monitor, which uses multiple linting tools to rate the code's quality. Please let me know if there are any comments or suggestions! -- Nikhil ___ Freetype-devel mailing list Freetype-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/freetype-devel
