Skip to content

Conversation

@saukymo
Copy link
Contributor

@saukymo saukymo commented Apr 15, 2022

No description provided.

@fulmicoton fulmicoton requested a review from ChillFish8 April 20, 2022 06:46
@fulmicoton
Copy link
Contributor

@ChillFish8 could you review?

@ChillFish8
Copy link
Collaborator

I'll try review this over the weekend, my computer's decided to call it quits right now so I'm unable to test this pr until then.

@saukymo
Copy link
Contributor Author

saukymo commented Apr 27, 2022

Formated with Rustfmt and pushed again. thx.

Comment on lines +28 to +32
JsonValue::Number(n) => match n {
n if n.is_i64() => n.as_i64().to_object(py),
n if n.is_u64() => n.as_u64().to_object(py),
n if n.is_f64() => n.as_f64().to_object(py),
_ => panic!("number too large"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I know this is slightly pedantic, but I'm pretty sure you can match on the num variant i.e Number::I64 which should remove the need for this panic. I think it'll be more idiomatic that way. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The enum inside Number is a enum of

enum N {
    PosInt(u64),
    /// Always less than zero.
    NegInt(i64),
    /// Always finite.
    Float(f64),
}

And it's a private field. So I don't figure out how to match them directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's a bit annoying. Oh well, happy to merge!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! But I don't have the write access. Please help to merge it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@ChillFish8 ChillFish8 merged commit ff51f61 into quickwit-oss:master Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants