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

Split matrix server selection into utility function #3427

Merged
merged 6 commits into from
Feb 8, 2019
Merged

Split matrix server selection into utility function #3427

merged 6 commits into from
Feb 8, 2019

Conversation

andrevmatos
Copy link
Contributor

@andrevmatos andrevmatos commented Feb 7, 2019

Follow-up of #3393 , implement some leftovers from that PR

@palango
Copy link
Contributor

palango commented Feb 7, 2019

I also don't see the send_global function. Did you forget to push that?

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1d90895). Click here to learn what that means.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3427   +/-   ##
=========================================
  Coverage          ?   76.05%           
=========================================
  Files             ?      100           
  Lines             ?    12960           
  Branches          ?     1824           
=========================================
  Hits              ?     9857           
  Misses            ?     2461           
  Partials          ?      642
Impacted Files Coverage Δ
raiden/network/transport/matrix/__init__.py 100% <ø> (ø)
raiden/network/transport/matrix/transport.py 75.2% <66.66%> (ø)
raiden/network/transport/matrix/utils.py 75.83% <87.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d90895...46a0397. Read the comment docs.

@andrevmatos andrevmatos changed the title Split matrix select_server, add send_global and monitoring rooms Split matrix server selection into utility function Feb 7, 2019
@andrevmatos
Copy link
Contributor Author

@palango the PR was still in WIP state, I would still add it. But as this was already getting a little big, decided to split the PR and do this only as the promised follow-up of #3393 . The send_global/MS PR will follow shortly

Copy link
Contributor

@rakanalh rakanalh left a comment

Choose a reason for hiding this comment

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

Looks good

raiden/network/transport/matrix/transport.py Show resolved Hide resolved
Copy link
Collaborator

@ulope ulope left a comment

Choose a reason for hiding this comment

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

Lgtm.

One question, below

available_servers = [config['server']]
else:
raise TransportError('Invalid matrix server specified (valid values: "auto" or a URL)')

def _http_retry_delay() -> Iterable[float]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn’t this be part of the utility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ulope this had too much to do with internals and variables of config, and I wanted to make the utility function as agnostic in relation to config as possible, that's why I kept this outside of it, but can be as easily used outside raiden as any other utility function, so we can say it's already split

raiden/network/transport/matrix/transport.py Show resolved Hide resolved
@andrevmatos andrevmatos merged commit 1b383ce into raiden-network:master Feb 8, 2019
@andrevmatos andrevmatos deleted the matrix_rooms branch February 8, 2019 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants