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

make json viewer more clear (reopen) #2350

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

make json viewer more clear (reopen) #2350

wants to merge 5 commits into from

Conversation

z44d
Copy link
Contributor

@z44d z44d commented Jul 21, 2024

reopened #2334

@coder2020official
Copy link
Collaborator

I propose:

  • Storing json_string, with 'from' being changed to 'from_user'
  • When printing an object, this json_string should be printed.

And then, we can add some JSON_FORMATTED = FALSE or something like this to types.

@Badiboy need your opinion. This way, users will be able to see a CLEAR response from Telegram.

@Badiboy
Copy link
Collaborator

Badiboy commented Jul 21, 2024

This way, users will be able to see a CLEAR response from Telegram.

Debug mode have full and clear responces of the Telegram.

  • Storing json_string, with 'from' being changed to 'from_user'

I have no willing to waste my and others server resources for storing json strings that I never used before and will never use in future for the only purpose that one guy after several years of library existence occasionally decided to have pretty output.

Except you will make it SO OPTIONAL that it will affect NOBODY except they obviously decided to use it.

@coder2020official
Copy link
Collaborator

I have no willing to waste my and others server resources for storing json strings

Makes sense

We still need changes to the output of the types though; Then, we can stick to the current implementation, but with pretty-printing optional.
I have personally encountered parts where outputting classes is SO annoying: 99% of values shown are None. I have to turn debug afterwards.
While debug is still needed, I see it reasonable to remove None values by default; And make pretty-printing optional by default.

@Badiboy
Copy link
Collaborator

Badiboy commented Jul 21, 2024

I have nothing against pretty output as I said from the very beginning which is:

  1. Optional
  2. Not consuming resources for those who do not need it

Intermediate version that was discussed that allowed:

  1. Optionally enable pretty output
  2. Optionally disable NONEs

Looked fine for me.

@coder2020official
Copy link
Collaborator

Is disabling Nones by default fine with you?

@Badiboy
Copy link
Collaborator

Badiboy commented Jul 21, 2024

Mmm... Let's be like that as far as Nones are really useless and it should be more or less clear that if you do not see property - it's empty.

@coder2020official
Copy link
Collaborator

Well I don't see any use case for seeing Nones. They are useless as API does not provide them.
They just take up space, nothing useful out of this.

@coder2020official
Copy link
Collaborator

Allright!
@2ei then could you please:

Create 2 variables:

  1. JSONDESERIALIZABLE_PARSE_OUTPUT = False
  2. JSONDESERIALIZABLE_SKIP_NONE = True

Use these to pretty print and output None values if necessary.

@z44d
Copy link
Contributor Author

z44d commented Jul 21, 2024

@coder2020official Done

Aslo, what about adding
JSONDESERIALIZABL_PARSE_INDENT = 2
too?

@coder2020official
Copy link
Collaborator

@Badiboy do you think parse indent is this necessary

@Badiboy
Copy link
Collaborator

Badiboy commented Jul 22, 2024

parse indent

What's that?

telebot/types.py Outdated
}
)
return (
json.dumps(self, default=default, indent=2, ensure_ascii=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

indent number here

@coder2020official
Copy link
Collaborator

@2ei just implement it, won't hurt

@coder2020official
Copy link
Collaborator

Thanks for the PR. Will test it out ASAP

telebot/types.py Outdated
}
return str(d)
def __str__(self) -> str:
default = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Frankly speaking, I do not understand anything in this pigeon talk. It takes too much time to decode.

Can this code be rewriten in adequate if / then / else mode? So ANYBODY who see it can understand what will be returned in what case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@Badiboy
Copy link
Collaborator

Badiboy commented Jul 30, 2024

@2ei Thank you.
LGTM if pass tests. )

@coder2020official
Copy link
Collaborator

Screenshot 2024-08-11 at 4 55 16 PM LGTM. Just one more think: @Badiboy do you think it would be better if this PR also removes json field in the output? or do you think it will be necessary? It is just a repetition of the whole dict.

@Badiboy
Copy link
Collaborator

Badiboy commented Aug 11, 2024

Romoving json looks reasonable: no need to duplicate the same data.

@z44d
Copy link
Contributor Author

z44d commented Aug 11, 2024

Romoving json looks reasonable: no need to duplicate the same data.

what should i do in your opinion?, convert json parameter to property method or just make an condition in JsonDeserializable ?

Also i just noticed that there is no type hint for json parameter, on my way to add it too.

@coder2020official
Copy link
Collaborator

LGTM. What did we agree on json?

@z44d
Copy link
Contributor Author

z44d commented Aug 31, 2024

Romoving json looks reasonable: no need to duplicate the same data.

what should i do in your opinion?, convert json parameter to property method or just make an condition in JsonDeserializable ?

Also i just noticed that there is no type hint for json parameter, on my way to add it too.

idk, i have 2 suggestions here, what is the best one for you @coder2020official ?

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

Successfully merging this pull request may close these issues.

3 participants