[herald] Move telegram function to telegram.js
ClosedPublic
Actions

Authored by bytesofman on Apr 14 2023, 03:21.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCb55ade9770e4: [herald] Move telegram function to telegram.js

Summary

T2972

This Telegram msg preprocessing function belongs in utils. Move it there.

Test Plan

npm test

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Errors

Severity	Location	Code	Message
Error	apps/ecash-herald/test/utilsTests.js:15	ESLINT	no-unused-vars

Unit

No Test Coverage

Build Status

Buildable 23319
Build 46257: Build Diff	ecash-herald-tests
Build 46256: arc lint + arc unit

Event Timeline

bytesofman created this revision.Apr 14 2023, 03:21

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 14 2023, 03:21

bytesofman requested review of this revision.Apr 14 2023, 03:21

Harbormaster completed remote builds in B23305: Diff 39719.Apr 14 2023, 03:24

bytesofman added a child revision: D13714: [herald] Clean up code commenting in parse.js.Apr 14 2023, 03:29

I agree this function doesn't belong to parse.js, but it is a telegram specific function so it belongs to telegram.js.

Also I don't think calling it from parse.js is a good api: it's much cleaner to have a method for sending the message that takes care of that. This means that you could have several messaging bots and don't have to reformat in the parse.js function for each of them.

This revision now requires changes to proceed.Apr 14 2023, 13:23

In D13713#305309, @Fabien wrote:

I agree this function doesn't belong to parse.js, but it is a telegram specific function so it belongs to telegram.js.

k will move it

Also I don't think calling it from parse.js is a good api: it's much cleaner to have a method for sending the message that takes care of that. This means that you could have several messaging bots and don't have to reformat in the parse.js function for each of them.

It would be cleaner to have a method for sending the message that simply takes the whole msg and prepares it for whatever bot is about to send it. However such a function needs to be much more sophisticated -- it needs to know which "<" characters are part of URLs and should not be escaped vs which "<" characters are supposed to be rendered as "<"

Would need to have an entire HTML parsing routine to figure this out. Even then, getBlockTgMessage has several hardcoded sections that depend on telegram's specific HTML parser. These would need to be replaced with some kind of parameter.

I don't think it's technical debt to have a collection of parser specific functions like getBlockTweet, getBlockSlackMsg, etc, that all take the same data inputs but work with the different rulesets of each bot. There are too many differences between each given parser to make a clean master function. Each parser will have different character limits, API restrictions, parsing language, escape characters...parseBlock needs to be "the" parsing function, but having distinct msg building functions will be necessary.

Moving the function to telegram.js -- need to refactor telegram.js to stop actually initializing the bot. This needs to be done in index.js and passed as a param for unit tests to work.

tg function lives in telegram.js

Harbormaster completed remote builds in B23319: Diff 39742.Apr 14 2023, 14:12

linting

bytesofman retitled this revision from [herald] Move utils function to utils to [herald] Move telegram function to telegram.js.Apr 14 2023, 14:13

Harbormaster completed remote builds in B23320: Diff 39743.Apr 14 2023, 14:32

In D13713#305363, @bytesofman wrote:

In D13713#305309, @Fabien wrote:

I agree this function doesn't belong to parse.js, but it is a telegram specific function so it belongs to telegram.js.

k will move it

Also I don't think calling it from parse.js is a good api: it's much cleaner to have a method for sending the message that takes care of that. This means that you could have several messaging bots and don't have to reformat in the parse.js function for each of them.

It would be cleaner to have a method for sending the message that simply takes the whole msg and prepares it for whatever bot is about to send it. However such a function needs to be much more sophisticated -- it needs to know which "<" characters are part of URLs and should not be escaped vs which "<" characters are supposed to be rendered as "<"

Would need to have an entire HTML parsing routine to figure this out. Even then, getBlockTgMessage has several hardcoded sections that depend on telegram's specific HTML parser. These would need to be replaced with some kind of parameter.

I don't think it's technical debt to have a collection of parser specific functions like getBlockTweet, getBlockSlackMsg, etc, that all take the same data inputs but work with the different rulesets of each bot. There are too many differences between each given parser to make a clean master function. Each parser will have different character limits, API restrictions, parsing language, escape characters...parseBlock needs to be "the" parsing function, but having distinct msg building functions will be necessary.