Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Add mobile client error handling #506

Merged
merged 3 commits into from
Aug 27, 2020
Merged

Add mobile client error handling #506

merged 3 commits into from
Aug 27, 2020

Conversation

Robert-Steiner
Copy link
Contributor

Summary:

  • remove unwrap in places where it is not safe tounwrap

@codecov
Copy link

codecov bot commented Aug 26, 2020 •
edited
Loading

Codecov Report

Merging #506 into master will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
- Coverage   57.33%   57.27%   -0.06%     
==========================================
  Files          68       68              
  Lines        3117     3120       +3     
==========================================
  Hits         1787     1787              
- Misses       1330     1333       +3
Impacted Files Coverage Δ
rust/xaynet-client/src/mobile_client/client.rs 0.00% <ø> (ø)
rust/xaynet-client/src/mobile_client/mod.rs 0.00% <0.00%> (ø)

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 6884b12...29b280f. Read the comment docs.

@Robert-Steiner
Copy link
Contributor Author

I did one more small change to perform_task. If the creation of the runtime fails, the current state of the client is returned along with the error. Before, I just returned the error which would mean we would have to create a new client

little-dude
little-dude reviewed Aug 26, 2020
View reviewed changes
rust/xaynet-client/src/mobile_client/mod.rs
Ok(runtime) => runtime,
// We don't want to loose the current client because of a runtime error.
// Therefore we return the error as well as the current client.
Err(err) => return Err((self, err.into())),
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmh I understand why we're doing that but it feels awkward. I think it would at least deserve an explanation in the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the documentaion for all public methods :
In addtion, I renamed two methods deserialize -> restore and perfrom_task -> try_to_proceed.

@Robert-Steiner Robert-Steiner merged commit 5a31b32 into master Aug 27, 2020
@Robert-Steiner Robert-Steiner deleted the mobile-client-error-handling branch August 27, 2020 08:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@little-dude little-dude little-dude 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.
2 participants
@Robert-Steiner @little-dude