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

Integer support #210

Merged
merged 1 commit into from May 5, 2020
Merged

Integer support #210

merged 1 commit into from May 5, 2020

Conversation

elrnv
Copy link
Contributor

@elrnv elrnv commented Apr 18, 2020

Roughly following the discussion from #47 and related issues and PRs, this PR extends the Number type (and so indirectly the Value type) to support integers. I hope I didn't grossly misunderstand the discussion there, but I am keen to make adjustments here to make the project better.

I tried to limit the impact to the API to a single breaking change by extending the already opaque Number type rather than the Value enum which exposes its structure. The breaking change is that the get method in Number is replaced by as_f64 and as_i64 for getting the appropriate type.
Since Integer and Float types are distinct, they will not be ordered by value (as explained below). This is a breaking change in itself.

To maintain the traits on Number, Integers are simply ordered before Floats. This makes float and integer not comparable in value (e.g. Number::Integer(2) < Number::Float(Float(1.0))) because I didn't see any necessity to do so, but I'm open to suggestions.

The tests are adjusted to verify that both integers and floats are working.

kvark
kvark approved these changes Apr 20, 2020
Copy link
Collaborator

@kvark kvark left a comment

This looks good to me, thank you!

src/value.rs Outdated
/// assert_eq!(i.get(), 5.0);
/// assert_eq!(f.get(), 2.0);
/// ```
pub fn get(self) -> f64 {
Copy link
Collaborator

@kvark kvark Apr 20, 2020

Choose a reason for hiding this comment

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

should be called into_f64

Copy link
Contributor Author

@elrnv elrnv Apr 21, 2020

Choose a reason for hiding this comment

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

Should we have a deprecated get that is an alias for into_f64 for backwards compatibility?

Copy link
Collaborator

@kvark kvark Apr 21, 2020

Choose a reason for hiding this comment

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

I thought your PR is API breaking anyway, isn't it?

Copy link
Contributor Author

@elrnv elrnv Apr 21, 2020

Choose a reason for hiding this comment

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

You're right. sorry, since floats are strictly not equal to integers of the same value. Do you think it's worthwhile to try to preserve this equality at all for compatibility or otherwise?

@kvark
Copy link
Collaborator

kvark commented Apr 20, 2020

@torkleyy would you want to review? I figured you know the value semantics better :)

kvark
kvark approved these changes Apr 21, 2020
Copy link
Collaborator

@kvark kvark left a comment

Thanks! We'll squash on merge.

@elrnv
Copy link
Contributor Author

elrnv commented Apr 26, 2020

The rebase is done. Is there anything else needed here from me?

@kvark
Copy link
Collaborator

kvark commented Apr 26, 2020

@elrnv no, thank you!
Let's ping @torkleyy again and give them a few days, then proceed.

@kvark
Copy link
Collaborator

kvark commented May 2, 2020

Sorry, this needs to be rebased again.
Otherwise, we are all good to go.

@elrnv
Copy link
Contributor Author

elrnv commented May 3, 2020

@kvark No problemo. Should be good now.
FYI I added a couple of From impls for the Number type that do proper (lossless) conversion from u64 and i32. Previously I just had u64 truncating to i64.

@kvark
Copy link
Collaborator

kvark commented May 3, 2020

@CAD97 looks like GHA isn't configured correctly. It doesn't even run on this PR.

@kvark
Copy link
Collaborator

kvark commented May 3, 2020

@elrnv thank you! Unfortunately we can't proceed until we get CI looking at this.

@kvark
Copy link
Collaborator

kvark commented May 3, 2020

Thank you! I think the CI fix need to go separately first.

@elrnv
Copy link
Contributor Author

elrnv commented May 3, 2020

I thought there was an option to run GHA on forks, but I can't find it anymore. I was just trying to see if this would work... :/

@CAD97
Copy link
Contributor

CAD97 commented May 3, 2020

@kvark I set it up the same way that Travis was, to only run on pushes to master/staging/trying. We can set it up to run on PRs as well, that's just another entry to the on key.

@kvark
Copy link
Collaborator

kvark commented May 3, 2020

@CAD97 travis was definitely checking other forks PRs

@CAD97
Copy link
Contributor

CAD97 commented May 4, 2020

#227 bumps up the CI to always run on any PR or any push. Bors would have (of course) still tested on CI.

@kvark
Copy link
Collaborator

kvark commented May 4, 2020

Can we rebase and squash please?

Extend the number value variant to distinguish between signed integers and floats.
Add a doc test for map_to on Number.
Added Number::Integer constructors from:
  - u64 since we get these for positive integers from the parser.
  - i32 since integers default to i32 in Rust, making the simple integer
    construction compile: Number::new(3) -> Number::Integer(3)
@CAD97
Copy link
Contributor

CAD97 commented May 5, 2020

.... I have no idea why GHA aren't running on this PR now. I can only suggest a bors try or recreating the PR at this point.

@kvark
Copy link
Collaborator

kvark commented May 5, 2020

I'll try to close and re-open it.

@kvark kvark closed this May 5, 2020
@kvark kvark reopened this May 5, 2020
@kvark
Copy link
Collaborator

kvark commented May 5, 2020

Doesn't seem to help :/

@kvark
Copy link
Collaborator

kvark commented May 5, 2020

Maybe related to:

Update - We are continuing to work on a fix for the elevated error rates.
May 5, 01:34 UTC
Update - We have identified the issue causing elevated error rates and are working on a fix.
May 5, 01:09 UTC
Investigating - We are investigating elevated errors with GitHub.
May 5, 00:45 UTC

@CAD97
Copy link
Contributor

CAD97 commented May 5, 2020

CI has run now, anyway.

@kvark
Copy link
Collaborator

kvark commented May 5, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented May 5, 2020

Build succeeded:

@bors bors bot merged commit 82d97ce into ron-rs:master May 5, 2020
5 checks passed
@elrnv elrnv deleted the integer-support branch May 5, 2020
@kvark kvark mentioned this pull request May 11, 2020
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.

None yet

3 participants