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

Matrix reporter #445

Merged
merged 3 commits into from Mar 25, 2020
Merged

Matrix reporter #445

merged 3 commits into from Mar 25, 2020

Conversation

@schildbach
Copy link
Contributor

schildbach commented Feb 2, 2020

I took the top three commits from https://github.com/reivilibre/fork-urlwatch-matrix/commits/master, squashed them and rebased on the last urlwatch release. Then I successfully verified that reporting via Matrix works.

I added my own corrections as separate commit(s). See the respective commit messages.

One problem I'm seeing is that the matrix-python-sdk dependency is not maintained any more (see their readme).

Note: I'm not a Python developer, so I only marginally reviewed the actual code.

@schildbach

This comment has been minimized.

Copy link
Contributor Author

schildbach commented Feb 2, 2020

I forgot to notify @reivilibre that his/her code is in this PR.

@schildbach

This comment has been minimized.

Copy link
Contributor Author

schildbach commented Feb 3, 2020

We need a max message length check. If messages exceed 65k bytes/chars, we get:

matrix_client.errors.MatrixRequestError: 413: {"errcode":"M_TOO_LARGE","error":"event too large"}

@thp

This comment has been minimized.

Copy link
Owner

thp commented Feb 15, 2020

@schildbach the check is already in there as MAX_LENGTH = 4096, right?

@schildbach

This comment has been minimized.

Copy link
Contributor Author

schildbach commented Feb 16, 2020

@schildbach the check is already in there as MAX_LENGTH = 4096, right?

It doesn't seem to be effective. As described in my last comment, I've had a large message fail.

I also see no reference to the MAX_LENGTH variable. The other reporters have a code block like this:

        if len(body_text) > self.MAX_LENGTH:
            body_text = body_text[:self.MAX_LENGTH]
@schildbach

This comment has been minimized.

Copy link
Contributor Author

schildbach commented Feb 16, 2020

I've just pushed a commit on top that adds the missing body trim.

Copy link
Owner

thp left a comment

See one more comments. Wonder if the text report and markdown reporter could share more code?

@thp

This comment has been minimized.

Copy link
Owner

thp commented Mar 23, 2020

@schildbach Had time to look at my comment? Other than that, I think the PR is ready to be merged.

@thp thp mentioned this pull request Mar 23, 2020
@thp
thp approved these changes Mar 25, 2020
@thp thp merged commit fcdc103 into thp:master Mar 25, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@thp

This comment has been minimized.

Copy link
Owner

thp commented Mar 25, 2020

Merged, thanks @schildbach and @reivilibre :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.