-
Notifications
You must be signed in to change notification settings - Fork 25
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
Stats: json format #22
Stats: json format #22
Conversation
/label ~Enhancement Edit: Ooops, I tried. |
ayyyy we broky |
aww crap. |
The latest commit on our master branch (100a7cc) it's causing merge conflicts. While we review and test your code, please resolve them. |
…via $TELEGRAM_STAT_HIDE_SENSIBLE_DATA=1, too.
3b880a8
to
b6961d2
Compare
# Conflicts: # README.md # telegram-bot-api/ClientManager.cpp
@luckydonald status? We can review it? |
Ah, yes I think so. |
Well if those |
I created dc397e0 with that new way. |
}; | ||
|
||
class JsonStatsBotStats : public td::Jsonable { | ||
public: | ||
explicit JsonStatsBotStats(const std::vector<ServerBotStat> stats) : stats_(stats) { | ||
explicit JsonStatsBotStats(td::vector<ServerBotStat> stats) : stats_(std::move(stats)) { | ||
CHECK(BotStatActor::SIZE == 4 && "Check that we have 4 fields."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& "Check that we have 4 fields."
doesn't make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Did I start a review on my own now?
jb_root.leave(); | ||
sb.clear(); | ||
} else { | ||
jb_root("⚠️ WARNING", "The json representation is still a work in progress and will be changed later!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol remove emoji
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your parser can't handle the unicode, you're out of luck with receiving text messages in telegram anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-ld so?
If this is the only issue you found i think is ok to merge, after all this
is more a visual warning for those who open the json from the browser than
an actual parameter to be read by a script
|
@giuseppeM99 @luckydonald merged. |
The work regarding #17.
With 374928c we could merge a working version where the json is still missing a few fields, to iterate from there.
It is available via the normal stats endpoint by calling
/json
as the path.Everything else will have the old text style.