Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pformat to config object #33

Merged
merged 6 commits into from
Mar 26, 2021
Merged

Add pformat to config object #33

merged 6 commits into from
Mar 26, 2021

Conversation

danr
Copy link
Contributor

@danr danr commented Mar 11, 2021

I sometimes find pprint.pformat suboptimal so it would be nice if I could drop-in replace it for snoop. This PR adds pp_pformat to the config object (and the snoop.install method) to override it with some other function.

It is also useful if you want to configure pprint.pformat, for example passing sort_keys=False.

@alexmojaki
Copy link
Owner

Thanks, I'm in favour of this. Something similar was requested in #13.

Please:

  1. Change the name from pp_pformat to just pformat which I think is easier.
  2. Set the default in Config.__init__ rather than write.
  3. Use pprintpp if it can be imported, i.e. https://github.com/wolever/pprintpp is installed.
  4. Otherwise, try importing prettyprinter. Let me know if you think the precedence should be swapper, they look like similar quality.
  5. Add some documentation.

@alexmojaki
Copy link
Owner

Oh and there's also https://github.com/panyanyany/beeprint. Why are they all equally good looking lol

@danr
Copy link
Contributor Author

danr commented Mar 11, 2021

Thanks for the feedback. The prettyprinter library seems like the highest quality from a quick glance.

The precedence I expect is:

  1. The user has explicitly passed pformat to the install method or config object
  2. If installed: prettyprinter.pformat.
  3. If installed: pprintpp.pformat.
  4. The standard pprint.pformat.

I'm a bit hesitant to this check for external libraries during runtime but it's your shot of course. Personally I will just be using my own pretty printer for now.

On the failing test case: I'm not sure what happens on 3.4, but all of the versions get really confused after the with snoop: block. Should I design the test a bit differently perhaps?

@danr
Copy link
Contributor Author

danr commented Mar 11, 2021

New commit pushed now. I am not sure how to test if the 3rd party printing libraries are there with the tox setup. Ideally both with and without 3rd party printing libraries should be tested.

@danr danr changed the title Add pp_pformat to config object Add pformat to config object Mar 11, 2021
@danr
Copy link
Contributor Author

danr commented Mar 11, 2021

I wrote:

On the failing test case: I'm not sure what happens on 3.4, but all of the versions get really confused after the with snoop: block. Should I design the test a bit differently perhaps?

I designed the test a bit differently, now this distractor is not present.

@danr
Copy link
Contributor Author

danr commented Mar 11, 2021

Ok, python 3.4 is still confused. Is this to be expected?

@alexmojaki
Copy link
Owner

pp is not expected to work in 3.4.

Use FIX_SNOOP_TESTS=1 tox to generate the sample files.

@alexmojaki
Copy link
Owner

all of the versions get really confused after the with snoop: block.

That really shouldn't happen, I will try finding out why later on.

@danr
Copy link
Contributor Author

danr commented Mar 11, 2021

Do you have some way to install all the python versions for tox? I'm installing python34 from arch linux aur now...

@alexmojaki
Copy link
Owner

pyenv is great, for me at least, not sure about ubuntu.

If you have difficulties, I'll handle the tests.

@danr
Copy link
Contributor Author

danr commented Mar 11, 2021

Hmm yes please do, I'm struggling getting a 3.4 working.

@alexmojaki
Copy link
Owner

alexmojaki commented Mar 12, 2021

The precedence I expect is:

  1. The user has explicitly passed pformat to the install method or config object
  2. If installed: prettyprinter.pformat.
  3. If installed: pprintpp.pformat.
  4. The standard pprint.pformat.

I'm a bit hesitant to this check for external libraries during runtime but it's your shot of course. Personally I will just be using my own pretty printer for now.

Add https://github.com/panyanyany/beeprint to that list.

Install all the libraries for tests. Test that it uses prettyprinter by default. Set sys.modules["prettyprinter"] = None (I haven't actually tested this but I'm sure something similar must be possible). Test that it now uses pprintpp by default, etc.

@alexmojaki
Copy link
Owner

Oh, beeprint is not just like pprint, forget that.

@alexmojaki
Copy link
Owner

I'm catching all exceptions rather than just ImportError because that's been an issue for me before: alexmojaki/birdseye#82

@alexmojaki
Copy link
Owner

all of the versions get really confused after the with snoop: block.

That really shouldn't happen, I will try finding out why later on.

Fixed in #34. This was a very weird and interesting problem, see cool-RR/PySnooper#214 for the explanation. Thanks for your contribution which picked this up.

@danr
Copy link
Contributor Author

danr commented Mar 15, 2021

Added a few lines of documentation and tests for the 3rd party libraries. Turns out you can simply do sys.modules['pprintpp'] = {} to make from pprintpp import pformat fail (since that empty dictionary does not contain {}).

Fixed in #34. This was a very weird and interesting problem, see cool-RR/PySnooper#214 for the explanation. Thanks for your contribution which picked this up.

Nice!

@danr
Copy link
Contributor Author

danr commented Mar 15, 2021

I only ran tests for 2.7 and 3.9 since that's the versions I have around 😬

If you want a custom column, please open an issue to tell me what you're interested in! In the meantime, you can pass a list, where the elements are either strings or callables. The callables should take one argument, which will be an `Event` object. It has attributes `frame`, `event`, and `arg`, as specified in [`sys.settrace()`](https://docs.python.org/3/library/sys.html#sys.settrace), and other attributes which may change.

If you want a custom column, please open an issue to tell me what you're interested in! In the meantime, you can pass a list, where the elements are either strings or callables. The callables should take one argument, which will be an `Event` object. It has attributes `frame`, `event`, and `arg`, as specified in [`sys.settrace()`](https://docs.python.org/3/library/sys.html#sys.settrace), and other attributes which may change.
- `pformat`: set the pretty formatting function `pp` uses. Default is to use the first of `prettyprinter.pformat`, `pprintpp.pformat` and `pprint.pformat` that can be imported.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also mention the pformat argument here and under Output configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is mentioned there (?) I pushed a new commit which adds it to the README's Output configuration section.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol my brain only saw mention of the libraries and not the argument.

@alexmojaki
Copy link
Owner

Had to take a detour to fix birdseye which was broken by a new release of sqlalchemy...

@alexmojaki alexmojaki merged commit 44d659a into alexmojaki:master Mar 26, 2021
@alexmojaki
Copy link
Owner

alexmojaki commented Mar 26, 2021

Ah, I think I meant to say that pformat needs to be mentioned here: https://github.com/alexmojaki/snoop#pp---awesome-print-debugging . But now that I think of it, there's no mention of configuration there, so it's fine.

Anyway, this is released under 0.3.0

Thank you!

@danr
Copy link
Contributor Author

danr commented Mar 27, 2021

Thanks Alex!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants