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.

Sign up for GitHub

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

Jump to bottom

Trim Markdown code block syntax #1275

Merged
merged 14 commits into from
Apr 7, 2021
Merged

Trim Markdown code block syntax #1275

merged 14 commits into from
Apr 7, 2021

Conversation

jaller94
Copy link
Contributor

@jaller94 jaller94 commented Mar 26, 2021 •
edited
Loading

Fixes #1271

Removes the code block syntax and adds the monospace character \x11 to the beginning of each line.

If the trimmed message is longer than the lineLimit, it still gets uploaded instead of posted directly.

Screenshot_2021-03-26_13-30-13

@jaller94 jaller94 changed the title Trim code block syntax [M->I]: Trim Markdown code block syntax Mar 26, 2021
@jaller94 jaller94 changed the title [M->I]: Trim Markdown code block syntax Trim Markdown code block syntax Mar 26, 2021
Half-Shot
Half-Shot reviewed Mar 26, 2021
View reviewed changes
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Heading in the right direction 👍 . The formatter looks good. Overall I wish our Matrix->IRC formatting code was less clunky (I would favour a pipeline of sorts) but I don't think we should go for a grand refactor here.

src/bridge/MatrixHandler.ts Outdated
@@ -956,6 +975,11 @@ export class MatrixHandler {
return;
}

const potentialCodeText = trimCodeMessages(text);
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that this isn't the best function to be doing this on. My wish is that we refactor this function to act as a pipeline of statements that modify a given event into an IRC message in a stateless context. Perhaps a better place would be IrcAction.fromMatrixAction, which is intended to be the point where we format a Matrix Event into an IRC action.

src/bridge/MatrixHandler.ts Outdated Show resolved Hide resolved
src/bridge/MatrixHandler.ts Outdated Show resolved Hide resolved
@jaller94
Copy link
Contributor Author

Known issues that remain:

  • If a Matrix user replies to a code block, the message gets uploaded. (with the default line limit of 3, because in the reply the code delimiters are not trimmed.

@jaller94 jaller94 requested a review from Half-Shot March 26, 2021 20:01
@Half-Shot
Copy link
Contributor

If a Matrix user replies to a code block, the message gets uploaded. (with the default line limit of 3, because in the reply the code delimiters are not trimmed.

This is surprising, because I believe we have our own reply format which only includes a truncated form of the original message. Would it be possible to fix that?

Half-Shot
Half-Shot reviewed Mar 29, 2021
View reviewed changes
src/bridge/MatrixHandler.ts Outdated
}

let cacheBody = text;
let cacheBody = event.content.body;
Copy link
Contributor

@Half-Shot Half-Shot Mar 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This changes the code so that we now cache the body, rather than trimCodeMessages(body), which seems like a potentially dangerous change? Maybe we should cache the text that we would be sending to IRC.

Half-Shot
Half-Shot reviewed Mar 29, 2021
View reviewed changes
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Some more thoughts

src/bridge/MatrixHandler.ts Outdated Show resolved Hide resolved
src/models/IrcAction.ts Show resolved Hide resolved
@jaller94 jaller94 requested a review from Half-Shot April 7, 2021 11:33
Half-Shot
Half-Shot approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Couple of nits, otherwise okay. CI looks unhappy btw

src/bridge/MatrixHandler.ts Outdated
@@ -964,8 +964,13 @@ export class MatrixHandler {
cacheBody = reply.reply;
}
}
let body = cacheBody.trim().substr(0, REPLY_SOURCE_MAX_LENGTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr is technically deprecated in favour of substring.

src/bridge/MatrixHandler.ts Outdated
@@ -964,8 +964,13 @@ export class MatrixHandler {
cacheBody = reply.reply;
}
}
let body = cacheBody.trim().substr(0, REPLY_SOURCE_MAX_LENGTH);
const nextNewLine = body.indexOf('\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to use " quotes here.

@jaller94 jaller94 force-pushed the j94/make-code-snippets-nicer branch from 0494cba to f9373e6 Compare April 7, 2021 12:32
@jaller94 jaller94 merged commit ab4f5c1 into develop Apr 7, 2021
@jaller94 jaller94 deleted the j94/make-code-snippets-nicer branch April 7, 2021 13:17
@progval
Copy link
Contributor

progval commented Apr 7, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Half-Shot Half-Shot Half-Shot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

``` should be stripped instead of triggering the "long message" mechanism
3 participants
@jaller94 @Half-Shot @progval