GeezerVison

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

This will break people who want to retain a string (ast.dump returns a string, it doesn't print something). Please don't do this but only do it for the CLI.

Ok, I made color=False the default.

I have made the requested changes; please review again

I reformulate what I said in the issue: it won't be possible to add colors to the CLI only as we would get a string only, so ok for this additional parameter. However, node that there are sometimes hidden attributes in the dump, so choose another color for them as well.

Actually, I don't know whether your approach is entirely fine. AST nodes can contain arbitrary strings and this can definitely mess the output. You can either (1) change how the formatted string is constructed (depending on color) [in which case you retain the color keyword] or (2) don't add a color keyword and only add it to the CLI and then colorize the entire string after it's been dumped with the regex.

Note: I'm not entirely sure we can have a perfect colorization during the formatting as we use repr. Though, we know that ast nodes's repr start with their class name so we can colorize the prefix properly. Colorizing at the end just for the CLI may seem ok though however we should make sure that we don't have ANSI control codes inside already, don't we?

Thanks for the review Bénédikt, I changed it to add color directly in _format.

Fuzz failure is unrelated, I think something must be messing with the path. I’ll look into it. CC @sethmlarson

btw we can now remove color=True from argparse.ArgumentParser(color=True) as it's the default (and in fact could remove from other CLIs).

I recommend replacing it with formatter_class=argparse.ArgumentDefaultsHelpFormatter to also include colour defaults in the help (and remove the hardcoded defaults in help=).

I did those too.

Does it make sense to support ast.dump(color=None) and have it either use or not use color depending on the env? Do we do that in other similar cases?

Does it make sense to support ast.dump(color=None) and have it either use or not use color depending on the env? Do we do that in other similar cases?

Hmm, we don't have a precedent, the other APIs with color support (e.g. difflib.unified_diff) are all True or False too.

I think it was a mistake to have color=True for unified_diff, or more generally for functions that return strings because they are actually breaking APIs that expect plain strings (possibly to render to something else afterwards that does not support colors at all)

As for ast.dump(), IMO it's best to just leave it as True/False.

Does it make sense to support ast.dump(color=None) and have it either use or not use color depending on the env? Do we do that in other similar cases?

Does it make sense? No.

ast.dump is env irrelevant. It it not attached to some file.

It makes sense if we have something like ast.print which accepts the file input.

Thanks all for the reviews!