Skip to content

Conversation

@EepyElvyra
Copy link
Contributor

About

This pull request removes the __repr__ and __str__ so they do not override the attrs repr

Checklist

  • The pre-commit code linter has been run over all edited files to ensure the code is linted.
  • I've ensured the change(s) work on 3.8.6 and higher.

I've made this pull request: (check all that apply)

This is:

  • A breaking change

@EepyElvyra EepyElvyra added bug Something isn't working enhancement New feature or request good first issue Good for newcomers labels Nov 5, 2022
@EepyElvyra EepyElvyra changed the base branch from stable to unstable November 5, 2022 21:25
@Catalyst4222
Copy link
Contributor

What about emojis and users?
I think that the repr of the emoji is helpful, just in the wrong place. Could you make it a property instead?

@EepyElvyra
Copy link
Contributor Author

What about emojis and users? I think that the repr of the emoji is helpful, just in the wrong place. Could you make it a property instead?

I don't see how making it a property is helpful? It is a very good thing that when you do "{emoji}" the emoji gets immediately converted to it's sendable form and I don't see a reason to change that

@Catalyst4222
Copy link
Contributor

First, fstrings convert using __str__ in that case, and emojis have __repr__. Second, it shouldn't be too much effort for the user to use an attribute/property. Your alternative (adding the __str__ method) would slightly reduce typing for users using the fstring, but it can add confusing to devs printing out objects, which was the cause of the linked issue

@EepyElvyra
Copy link
Contributor Author

First, fstrings convert using __str__ in that case, and emojis have __repr__. Second, it shouldn't be too much effort for the user to use an attribute/property. Your alternative (adding the __str__ method) would slightly reduce typing for users using the fstring, but it can add confusing to devs printing out objects, which was the cause of the linked issue

I have changed it to __str__. I understand the confusion as I encountered it myself, too. However I think in case of the emoji object this confusion is at a lower level. I have enabled the attrs repr so if any developers are still highly confused about the emoji object printout they can print(repr(emoji))

@Catalyst4222
Copy link
Contributor

Many developers (including me) will print an object instead of the repr of the object, often because they're the same

What's the reason not to have it as a property?

@EepyElvyra
Copy link
Contributor Author

Many developers (including me) will print an object instead of the repr of the object, often because they're the same

What's the reason not to have it as a property?

It is an unnecessary breaking change imo.

Also, in most cases you won't bother about the attributes of an emoji, would you? I'd assume mostly you just want to send it or create an reaction with it. I really thing having emojis like that is fine. If major complaints emerge, we still can change it at a later point.

Copy link
Contributor

@Catalyst4222 Catalyst4222 left a comment

Choose a reason for hiding this comment

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

A complain already has emerged, which was the reason behind the issue in the first place. Having the magic method just adds unexpected cases, while a property will only impact code when it's accessed intentionally. A property makes it more clear what code is doing, which __str__ does not

@EepyElvyra EepyElvyra changed the title refactor: remove all unwanted __repr__ and __str__ refactor!: remove all unwanted __repr__ and __str__ Nov 5, 2022
FINE TAKE IT ITS GONE ARE YOU HAPPY NOW :MADGE:
@Catalyst4222 Catalyst4222 merged commit c7ccc02 into interactions-py:unstable Nov 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[REQUEST] Remove or improve __repr__ and __str__ on models

2 participants