Skip to content

Allow attributes to hide themselves in the repr() when they are the default #453

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

Closed
wants to merge 2 commits into from

Conversation

ssbr
Copy link

@ssbr ssbr commented Oct 20, 2018

The use case is something like: while it is often useful to have the repr() be "complete", it is also often useful for it to be "minimal but unambiguous". If you have 6 attributes but all but 1 are the default value, the signal might be lost in the noise, especially when those attributes themselves are attrs-based classes and the whole tree of objects takes up a few hundred bytes of repr().

The way this was implemented was to change repr from a bool to an int, which can be 0 (never show a repr), 1 (always show a repr) or 2 (only show a repr if the attribute has a non-default value). I wasn't sure if I should properly add an enum for this or a new parameter or what, so rather than fiddling with it on my own I am mailing this out as an MVP and asking for feedback.

Re pull request checklist below:

  • I cannot underestand tests/strategies.py or where I would add this.
  • I don't know what version I am supposed to use for versionadded/versionchanged.
  • I don't know what I'm supposed to put in changelog.d. It says to add a file with the PR#, but I haven't submitted the PR yet. I guess I will do this after I create the pull request.

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

ssbr added 2 commits October 20, 2018 14:04
…efault.

The use case is something like: while it is often useful to have the repr() be "complete", it is also often useful for it to be "minimal but unambiguous". If you have 6 attributes but all but 1 are the default value, the signal might be lost in the noise, especially when those attributes themselves are attrs-based classes and the whole tree of objects takes up a few hundred bytes of repr().

The way this was implemented was to change repr from a bool to an int, which can be 0 (never show a repr), 1 (always show a repr) or 2 (only show a repr if the attribute has a non-default value). I wasn't sure if I should properly add an enum for this or a new parameter or what, so rather than fiddling with it on my own I am mailing this out as an MVP and asking for feedback.
@hynek
Copy link
Member

hynek commented Oct 24, 2018

Two things:

One lingering topic we had in attrs is the move from booleans to Enums. Integers are a rather bad way to signal anything to APIs. So far I couldn’t make up my mind about whether to use enum and add a conditional enum34 dependency on Python 2 or some other shenanigans (but the closer 2020 comes, the more I think this is the way to go).

However, while your use case is interesting, it still a bit too specific and your approach lacks the flexibility to accommodate other needs. Have you seen #212? I think we should pursuit the Union[bool, Callable] approach in this case for now because it gains us much more.

@ssbr
Copy link
Author

ssbr commented Oct 31, 2018

Sorry for delay in responding, stuff has been busy. :(

The comments in #212 suggest that it should be Callable[[T], str] which wouldn't work for this. If instead we could use Union[bool, Callable[[T], Optional[str]], then I think that'd be perfect. Something like:

repr=False is equivalent to repr=lambda _: None
repr=True is equivalent to repr=repr

I can try to implement that if that sounds good to you. (Can either close this PR and open a new one, or reuse it, not sure what you'd prefer.)

(P.S. agree on ints, this API was ugly and more of a conversation starter, since I also wasn't sure if you were OK with enum etc. #212 is probably a better approach for sure.)

@hynek
Copy link
Member

hynek commented Nov 24, 2018

Sorry for delay in responding, stuff has been busy.

Oh god don't apologize; I'm barely keeping myself ATM. Thank you for hanging in there!

I would say that you should just open a new one, to have a fresh discussion.

We could probably add something like attr.DROP_IF_DEFAULT 🤔

@Julian
Copy link
Member

Julian commented Sep 22, 2019

So now that #212 is closed... maybe worth another look at this?

Though personally.. my first intuition is that this belongs in attr.s, not attr.ib [which we explicitly decided to push off on implementing in #212]. But maybe others feel differently.

(To speak the thought out more fully: I want to just write @attr.s(repr=attrs.minimal_repr(repr=reprlib.repr)) once, not have to pass some enum thing into 5 different attr.ibs that I want to not show if they're at their default vals)

@hynek
Copy link
Member

hynek commented Sep 22, 2019

Yes, my current plan is to make all the init, repr etc arguments optinally a callable that gets a tuple of Attributes and returns 1–4 methods that get attached to the class. It's so obvious and simple in hindsight, that it hurts.

I just need to work off some preparations (one of them was the eq/order split). I'm having a run ATM, so I hope I'll be able to finish that work in the next weeks (famous last words).

@hynek
Copy link
Member

hynek commented Sep 22, 2019

That said, I'm gonna close this, because that will be much easier once we've done the work I've outlined above!

Sorry and thanks!

@hynek hynek closed this Sep 22, 2019
@astrojuanlu
Copy link

This would have been a useful addition - similar to the print_changed_only setting in scikit-learn.

Yes, my current plan is to make all the init, repr etc arguments optinally a callable that gets a tuple of Attributes and returns 1–4 methods that get attached to the class. It's so obvious and simple in hindsight, that it hurts.

Is there any follow-up issue we can subscribe to?

@hynek
Copy link
Member

hynek commented Dec 28, 2020

Is there any follow-up issue we can subscribe to?

Unfortunately there currently isn't.

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.

4 participants