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

feat: Define StatusCode to indicate the result of sync operation #1856

Merged
merged 7 commits into from Feb 7, 2020

Conversation

@keroro520
Copy link
Contributor

keroro520 commented Dec 3, 2019

Learned from HTTP Response, use StatusCode to indicate the result of sync-operation, try to replace original Result<T, future::Error>.
More detail: https://github.com/keroro520/ckb/blob/a66ab9a7d5eec69211a9a5073ca32691113a0c2e/sync/src/status.rs#L31-L53

@keroro520 keroro520 requested a review from nervosnetwork/ckb-code-review as a code owner Dec 3, 2019
@doitian doitian added this to 👀 Awaiting review in CKB - Pull Requests Dec 4, 2019
@doitian doitian requested review from nervosnetwork/ckb-dev, quake and doitian and removed request for nervosnetwork/ckb-dev Jan 10, 2020
@doitian doitian requested review from zhangsoledad and removed request for quake Feb 5, 2020
sync/src/status.rs Outdated Show resolved Hide resolved
@zhangsoledad

This comment has been minimized.

Copy link
Member

zhangsoledad commented Feb 6, 2020

LGTM

@keroro520 keroro520 force-pushed the keroro520:sync-status branch from 28784e0 to 171c121 Feb 6, 2020
@keroro520 keroro520 requested a review from zhangsoledad Feb 6, 2020
}};
}

/// StatusCodes indicate whether a specific operation has been successfully completed.

This comment has been minimized.

Copy link
@doitian

doitian Feb 6, 2020

Member

It's a bit too complex. I prefer only two levels: the class and the code.

We can put the codes which are not serious in several dedicated classes, such as informational and warning. Following is an example list:

  • Informational
  • Warning
  • Remaining errors which can be classified by modules, such as general errors, data errors, consensus errors.

The code name can be more verbose and following some pattern, like CompactBlockRequiresParent, CompactBlockIsStaled

This comment has been minimized.

Copy link
@keroro520

keroro520 Feb 6, 2020

Author Contributor

Thanks for your suggestions.
The last 2 commits make updates based on the above suggestions:

  • Category status into 3 classes, informational(1xx), malformed error(4xx), warning(5xx)
  • Rename the code with verbose style. I also agree that a verbose name brings more readability. Please help review it again.
@doitian
doitian approved these changes Feb 7, 2020
CKB - Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved Feb 7, 2020
@doitian

This comment has been minimized.

Copy link
Member

doitian commented Feb 7, 2020

bors r=doitian,zhangsoledad

bors bot added a commit that referenced this pull request Feb 7, 2020
Merge #1856
1856: feat: Define StatusCode to indicate the result of sync operation r=doitian,zhangsoledad a=keroro520

Learned from HTTP Response, use `StatusCode` to indicate the result of sync-operation, try to replace original `Result<T, future::Error>`.
More detail: https://github.com/keroro520/ckb/blob/a66ab9a7d5eec69211a9a5073ca32691113a0c2e/sync/src/status.rs#L31-L53

Co-authored-by: keroro520 <keroroxx520@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Feb 7, 2020

Build failed

  • continuous-integration/travis-ci/push
@keroro520 keroro520 merged commit 26e4837 into nervosnetwork:develop Feb 7, 2020
4 of 5 checks passed
4 of 5 checks passed
bors Build failed
Details
Dummy CI CI that does nothing
Details
Travis CI - Pull Request Build Passed
Details
nervosnetwork.ckb Build #20200206.2 succeeded
Details
nervosnetwork.ckb (UnitTest) UnitTest succeeded
Details
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

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