Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix "list index out of range" error #4937

Closed
wants to merge 1 commit into from
Closed

Conversation

jo-so
Copy link

@jo-so jo-so commented Mar 26, 2019

Fixes #4842

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

Signed-off-by: Jörg Sommer <[email protected]>

Check if the batch contains events before trying to access them.

Fixes matrix-org#4842
@jo-so jo-so changed the base branch from master to develop March 26, 2019 12:11
@anoadragon453
Copy link
Member

Hello! Before review you'll need to add a changelog entry as per https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#changelog. Thanks!

@jo-so
Copy link
Author

jo-so commented Mar 29, 2019

Hello! Before review you'll need to add a changelog entry as per https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#changelog. Thanks!

And if the review gives a NACK?

The process of

  1. creating a commit
  2. push the branch
  3. create a pull-request
  4. create a doc-commit with the PR number
  5. repush the branch
  6. get a review which NACKs the PR or requests changes
  7. redo the doc stuff
    is really not handy, nor gives any appeal to do it.

@anoadragon453
Copy link
Member

@jo-so Fair enough.

@@ -770,7 +770,7 @@ def compute_state_delta(self, room_id, batch, sync_config, since_token, now_toke
current=current_state_ids,
lazy_load_members=lazy_load_members,
)
elif batch.limited:
elif batch.limited and batch.events:
Copy link
Member

Choose a reason for hiding this comment

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

To me it sounds like the error in #4842 was due to batch.events being an empty list, rather than None. In that case, this if statement would still run and crash.

Instead you may want to check if len(batch.events) > 0.

Copy link
Author

Choose a reason for hiding this comment

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

@richvdh
Copy link
Member

richvdh commented Apr 4, 2019

Hello! Before review you'll need to add a changelog entry as per https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#changelog. Thanks!

And if the review gives a NACK?

... then you update your PR accordingly.

The process of

  1. creating a commit
  2. push the branch
  3. create a pull-request
  4. create a doc-commit with the PR number
  5. repush the branch

you might find something like this useful to get the next PR number, so you can create the news file before pushing:

curl -s "https://api.github.com/repos/matrix-org/synapse/issues?state=all&per_page=1" | jq -r ".[0].number + 1"

Obviously it assumes you create the PR before any more issues or PRs are opened, but in general it will save time.

  1. get a review which NACKs the PR or requests changes
  2. redo the doc stuff

There is no need to redo the news file when updating a PR.

is really not handy, nor gives any appeal to do it.

I understand that it's not particularly convenient, but ultimately we struggle to handle the number of PRs we get as it is. Anything that contributors can do to make the job easier is much appreciated by us.

On which note, please ensure that your PRs contain a link to the bug that they are fixing in the body of the PR description. Github does not link to issues when the issue number is only in the title of the PR.

@richvdh richvdh changed the title Fix #4842 Fix "list index out of range" error Apr 4, 2019
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

please create the changelog file so that the CI runs

@richvdh
Copy link
Member

richvdh commented May 9, 2019

I'm not quite sure if this is the right solution. On hold pending further information at #4842 (comment).

@neilisfragile neilisfragile assigned richvdh and unassigned richvdh May 9, 2019
@richvdh richvdh self-requested a review May 15, 2019 15:46
@richvdh
Copy link
Member

richvdh commented May 15, 2019

I don't think this change is correct, and as per #4842 it's unclear what we're trying to fix anyway, so I'm going to close this for now.

@richvdh richvdh closed this May 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants