t3rn0 / ast-comments

Extension to the built-in ast module. Finds comments in source code and adds them to the parsed tree.
MIT License
31 stars 10 forks source link

Preserving blank lines? #18

Closed dvarrazzo closed 1 year ago

dvarrazzo commented 1 year ago

Hello,

I am experimenting with this module to auto-generate sync modules from async modules, see https://github.com/psycopg/psycopg/commit/16c2276a12fe9260df188956c57d51086a0235cf.

After a roundtrip from code to ast and back to code, the vertical blank line are lost.

Do you think it's possible to improve ast-comments to reproduce the same amount of blank lines in the code emitted?

Thank you very much!

dvarrazzo commented 1 year ago

I wrote a small proof of concept as an ast tree visitor to scan the tree looking for statements in a block separated by blank lines and adding blank comments at https://github.com/psycopg/psycopg/commit/b1fc524626997a55b5ac48ca2a6475e302ca4f87#diff-1a44aa6d70e4f303f3dcde568176d8c3525814e5d623309423fe1805f215f2dbR161

It's not perfect but, together with a pass of black, it does a decent enough job.

t3rn0 commented 1 year ago

Hello, Thank you for your interest in the library. I'm not sure what you are trying to do here. Can you give some examples please?

dvarrazzo commented 1 year ago

In psycopg 3 codebase there is a lot of duplicate code between sync and async object (both in the module and in the test suite). I am looking into auto-generating the common parts (basically, taking the AST of the async code and remove all the async/await construct, then making other small adjustments).

I am trying to keep the changes to the generated code minimal, and would like the generated sync code to be as readable as the async part:

So, apparently I can do something on our side already, using an unmodified version of ast-comments, in order to try to preserve vertical blanks in a roundtrip source -> ast -> source. However, maybe it would be useful for your library to add this feature.

t3rn0 commented 1 year ago

Thank you for the details About the BlanksInserter: it is not entirely clear why just one blank line is sufficient. Why not try keeping all blank lines instead?

I'm not sure ast-comments is supposed to do anything other than parsing/unparsing comments. And blank lines are something else. So I guess you'll have to keep the changes on your side.

Maybe we should think about creating another library that takes into account empty lines? If there are other forms of "non-ast" information that need to be parsed and stored, we might consider a plugin-based library (like flake8). Could be fun 🙂

Also, have you considered trying libcst for the problem?

dvarrazzo commented 1 year ago

Hello,

thank you for considering this change, and no worries if you don't think it is an appropriate improvement for your library. As I am running a visitor on my own side in order to adjust the whitespace, I don't require this operation to be performed into ast-comments.

The reason to insert only one space is that, when I didn't, too many whitespaces were inserted. I didn't debug precisely why, it's probably related to the presence of several nodes on the same line, which should be kept in consideration in other ways. The solution of inserting only one space is a temporary hack (which, together with black, manages to reconstruct the original state anyway) and I might improve it later.

I hadn't found LibCST, although I've been googling for things like that (looks like there are dozens of AST-related libraries around...). I will consider using it.

Thank you, have a good day!