I can't recommend black

[Edward K. Ream]

Black does a lot of things right, but imo it must not change the indentation of comments. Unless this is fixed, I shall not blacken Leo's code base.

I used two kinds of comments throughout Leo:  leading comments and trailing comments.  Leading comments apply to the following line or lines:

# Compute g.app.signon.
signon = ['Leo %s' % leoVer]
...

Trailing comments apply only to the immediately preceding line:

self.batchMode = False
    # True: run in batch mode.
self.debug = []
    # A list of switches to be enabled.
self.diff = False
    # True: run Leo in diff mode.

Imo, dedenting these comments is simply wrong.

Next steps

I'll raise this issue with the black devs, after doing my homework, which will be to suggest a patch.  The last time I looked, handling whitespace in comments and strings was difficult.

If necessary, I'll copy a patched (forked) version of black to leo/external and use that for Leo's black command.

Another possibility: support black-like line breaks in Leo's legacy beautify commands. This is #1266.

Summary

Black should keep its hands off comments.

Workarounds include a fork of black, or using black-like line breaking in Leo's token-based beautify commands.

Edward

[Edward K. Ream]

On Saturday, September 7, 2019 at 6:17:35 AM UTC-5, Edward K. Ream wrote:

Black does a lot of things right, but imo it must not change the indentation of comments.

Heh.  There is simple workaround:  Put @nobeautify in nodes containing trailing comments.

A regex would have trouble discovering trailing comments.  A short script should do the trick.

Edward

[Edward K. Ream]

On Saturday, September 7, 2019 at 6:22:05 AM UTC-5, Edward K. Ream wrote:

>> Black does a lot of things right, but imo it must not change the indentation of comments.

> A regex would have trouble discovering trailing comments.  A short script should do the trick.

Or maybe even easier, just look at the diffs and put @nobeautify in the affected nodes.

Edward

[rengel]

self.batchMode = False
    # True: run in batch mode.
self.debug = []
    # A list of switches to be enabled.
self.diff = False
    # True: run Leo in diff mode.

Imo, dedenting these comments is simply wrong.

 Wouldn't it be better to do without comments? Imo, its a problem of naming the variables:

 

self.runInBatchMode = False
# or self.batchModeOn = False
self.debugSchwitches = []
self.runInDiffMode = False
# or self.diffModeOn = False

No need to remember what these variables mean.

No need for the comments in the first place.

Instead of fighting and adapting foreign tools it might be easier to make variable names self explaining.

Of course, I haven't checked all the instances where this might be applicable. But adapting third party tools to one's own bidding? Do you really want to do this every time such a tool gets updated?

 

 Reinhard

[jkn]

Hmm - I can't say as I'd ever grokked this 'indented trailing comment' as an actual style within Leo. I probably thought it was some old formatting cruft when I saw it.

It is used in other substantive codebases, do you know?

[Edward K. Ream]

On Sat, Sep 7, 2019 at 3:50 PM jkn <jkn...@nicorp.f9.co.uk> wrote:

[Are trailing comments] used in other substantive codebases, do you know?

I first saw it in Emacs, iirc, or maybe it was the gnu C compiler.

Edward

[Edward K. Ream]

On Sat, Sep 7, 2019 at 11:30 AM rengel <reinhard...@gmail.com> wrote:

Wouldn't it be better to do without comments? IMO its a problem of naming the variables:

So you're saying that I/we should rename dozens/hundreds of ivars and globals to work around black's notions?

Black is a tool, not a straitjacket.

Edward

[rengel]

On Saturday, September 7, 2019 at 11:07:31 PM UTC+2, Edward K. Ream wrote:

...

So you're saying that I/we should rename dozens/hundreds of ivars and globals to work around black's notions?

Sorry! I'm not saying that you (or anybody else) should do anything.

I'm saying that if variable names are self-explanatory then one doesn't need these comments. To do this just to please black (or any other tool, for that matter) would be foolish.

But to answer your question: If it were my code base I would rename dozens/hundreds of ivars, if and only if this would improve (= make it more readable, shorter) the code.

In a former life, I translated computer literature into German, among them such pearls like Clean Code by Robert C. Martin and Working Effectively with Legacy Code by Michael C. Feathers. They can teach a lot about naming strategies, restructuring code and elemination of comments to improve a code base.

My experience taught me never to work against a tool I don't have control of. A couple of months down the road I tend to forget why I used this or that workaround just to make the tool work. See your own comment in 'Orange is the new black; 'I spent several hours this morning trying, and utterly failing to alter black's code to do anything new. '

Reinhard

[Edward K. Ream]

On Sun, Sep 8, 2019 at 12:48 AM rengel <reinhard...@gmail.com> wrote:

> My experience taught me never to work against a tool I don't have control of. A couple of months down the road I tend to forget why I used this or that workaround just to make the tool work.

Recall that my intention was to propose a pull request to the black devs.  That was the purpose of yesterday's studies. Alas, I was not able to make any progress. Handling comments in an ast is inherently difficult.

Edward