repr for expressions

[volokluev]

Problem

AST printed is unreadable which makes test debugging difficult

Solution

Create a visitor class that traverses an expression and creates a string somewhat resembling the query. Use that to implement the repr method of the Expression base class

Results

Formatted queries look something like this:

f1(
  t1.c1,
  t1.c2
)(
  'hello',
  'kitty'
)

Test Plan

All types of visited nodes are tested, I am assuming that is sufficient, happy to add more if folks would like it

[evanh]

Per our discussion on Slack, I don't think this is more readable than what we have before (except that the indenting is nicer). {'__name__': 'Column', 'alias': None, 'column_name': 'c2', 'table_name': 't1'} is more readable than Column(alias='alias', table_name='table', column_name='column') and it's certainly not less verbose.

[evanh]

💀

[evanh]

Why do you not have a repr on these anymore?

[volokluev]

dataclasses have a default repr, you need to explicitly say you're using your own so that it uses your implementation

[evanh]

This looks pretty good to me, well done. Just one comment on the tests.

[evanh]

You're missing a test for aliases.

[fpacifici]

Thanks, this looks good.
Just one concern on subscriptable references, I think it is missing an accept call on the key.

The rest are only questions or nits.
Do you plan to provide a more concise implementation for the str method or are we going to use the same ?

[volokluev]

Do you plan to provide a more concise implementation for the str method or are we going to use the same ?

I was planning on using the same one (at least for now). We can decide if we want anything different later

[volokluev]

Do you plan to provide a more concise implementation for the str method or are we going to use the same ?

Did you have something in mind? Maybe the repr but no newlines or indentation?

[fpacifici]

Did you have something in mind? Maybe the repr but no newlines or indentation?

Not really. I wondered what your plan was. I am ok with using the same representation.

[codecov-commenter]

Codecov Report

Merging #2020 (47adb52) into master (16ca215) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 47adb52 differs from pull request most recent head c429147. Consider uploading reports for the commit c429147 to get more accurate results

@@            Coverage Diff             @@
##           master    #2020      +/-   ##
==========================================
+ Coverage   90.94%   91.02%   +0.07%     
==========================================
  Files         499      499              
  Lines       21499    21550      +51     
==========================================
+ Hits        19553    19615      +62     
+ Misses       1946     1935      -11     

Impacted Files	Coverage Δ
snuba/query/expressions.py	94.25% <100.00%> (+2.06%)	⬆️
tests/query/test_expressions.py	100.00% <100.00%> (ø)
snuba/cli/bootstrap.py	3.44% <0.00%> (+3.44%)	⬆️
snuba/cli/api.py	43.47% <0.00%> (+26.08%)	⬆️
snuba/cli/__init__.py	100.00% <0.00%> (+30.00%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16ca215...c429147. Read the comment docs.

[fpacifici]

This is great, thanks.

please remove the print statement before merging, and would you mind merging tomorrow so we can go together through a deployment ?