search

cool-RR / PySnooper

Never use print for debugging again
MIT License
16.34k stars 951 forks source link

Feature/normalize output #169

Closed itraviv closed 4 years ago

itraviv commented 4 years ago

normalizing output - removing machine specific data:

tests:

cool-RR commented 4 years ago

Good job. Before I do a thorough code review, please take care of these issues:

  1. Your code shouldn't be changing whitespace of any lines that aren't related to your code. Go over the "Files changed" in this PR and you'll see that there are a bunch of changes like that.
  2. I see that you check whether the default Python repr was used by using a regular expression. That's an okay solution, but if you can instead do a check that doesn't rely on regexp, that'd be better.
  3. It would be nicer if your commits were more finalized (probably less commits, clearer messages), but I won't hold up your PR for that, worst-case scenario I'll organize them later.

I see that you closed your PR, maybe you're opening another one or something, I'll stay tuned.

itraviv commented 4 years ago

i closed it because it failed the CI. i will fix and organize the commits as you requested. stay tuned for an improved version.

cool-RR commented 4 years ago

Cool. You could keep it open if you want though and keep updating it until the CI passes, it's all good. You can also run tests locally with pytest, I think that just running pytest will do the trick.

On Tue, Nov 19, 2019 at 1:58 PM itraviv notifications@github.com wrote:

i closed it because it failed the CI. i will fix and organize the commits as you requested. stay tuned for an improved version.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cool-RR/PySnooper/pull/169?email_source=notifications&email_token=AAAN3SW57IBMIDHNXGTLZVTQUPIHHA5CNFSM4JPB55D2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEN6KDI#issuecomment-555476237, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAN3SQBBDXIZRUOMTYK5BDQUPIHHANCNFSM4JPB55DQ .

itraviv commented 4 years ago

yea, pytest is not the issue here (i have also added tests and made sure everything passes), it's the python2.7 compatibility that fails. as for the regex - i couldn't find a better way since the number of figures appearing in the memory address (after the 0x..) is very platform and context specific (memory address range). if you have any other suggestion - please share with me.

cool-RR commented 4 years ago

Hmm, I think it's possible to figure out whether the default repr is used, here's some inspiration under Python 3, you could probably do something similar in Python 2:

In [15]: class A: 
    ...:     pass 
    ...: 

In [16]: class B(A): 
    ...:     def __repr__(self): return 'foo' 
    ...: 

In [17]: a.__repr__ 
Out[17]: <method-wrapper '__repr__' of A object at 0x0000000004D0D748> 

In [18]: a.__repr__.__objclass__ 
Out[18]: object 

In [19]: b.__repr__ 
Out[19]: <bound method B.__repr__ of foo> 

In [20]: b.__repr__.__func__ 
Out[20]: <function __main__.B.__repr__(self)> 

In [21]: b.__repr__.__func__.__qualname__ 
Out[21]: 'B.__repr__'

Another thought though: Maybe there are non-default reprs which also use the memory address. I actually think your regex method might be superior, so let's go with that. I'm keeping my above code here for reference in case we'll want it later.

itraviv commented 4 years ago

opened new PR with all mentioned changes: https://github.com/cool-RR/PySnooper/pull/170