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

Change error type produced by duration parser #5219

Merged
merged 11 commits into from Aug 7, 2019

Conversation

@iosmanthus
Copy link
Contributor

commented Aug 6, 2019

What have you changed? (mandatory)

Change the error type produced by Duration which make the conversion of Duration easier.

What are the type of changes? (mandatory)

The currently defined types are listed below, please pick one of the types for this PR by removing the others:

  • Improvement (a change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

No need to test. cargo check --all-targets

iosmanthus and others added some commits Aug 6, 2019

Make `codec::Error` more ergonomic
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
remove generic parameters
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Change error type return by `Duration::*`
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
Merge branch 'change-error-type-in-duration-parser' of github.com:ios…
…manthus/tikv into change-error-type-in-duration-parser

@iosmanthus iosmanthus added the C: Copr label Aug 6, 2019

@iosmanthus iosmanthus requested review from breeswish and lonng Aug 6, 2019

@breeswish

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

CI failed

Fix clippy warnings
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>
@lonng

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

/run-all-tests

@iosmanthus iosmanthus requested a review from breeswish Aug 7, 2019

hour,
MAX_HOURS
))
Err(Error::overflow(hour, MAX_HOURS))

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 7, 2019

Contributor

I find the Error::overflow code is:

pub fn overflow(data: impl Display, expr: impl Display) -> Error {
    let msg = format!("{} value is out of range in '{}'", data, expr);
    Error::Eval(msg, ERR_DATA_OUT_OF_RANGE)
}

It doesn't seem correct to me that MAX_HOURS is what the expr is expected to be here.

@@ -449,7 +437,7 @@ impl Duration {
Duration::from_micros(
millis
.checked_mul(1000)
.ok_or(invalid_type!("micros overflow"))?,
.ok_or_else(|| Error::overflow(millis, MAX_TIME_IN_SECS * 1000))?,

This comment has been minimized.

Copy link
@sticnarf

sticnarf Aug 7, 2019

Contributor

ditto

replace Error::Eval(.., OUT_OF_RANGE) with Error::overflow
Signed-off-by: Iosmanthus Teng <myosmanthustree@gmail.com>

@iosmanthus iosmanthus requested a review from lonng Aug 7, 2019

@lonng

lonng approved these changes Aug 7, 2019

@sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

/run-all-tests

@sre-bot sre-bot merged commit 1637c4e into tikv:master Aug 7, 2019

5 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.