Skip to content

Add support for number ranges #602

Open
Ztry8 wants to merge 52 commits into
ron-rs:masterfrom
Ztry8:master
Open

Add support for number ranges #602
Ztry8 wants to merge 52 commits into
ron-rs:masterfrom
Ztry8:master

Conversation

@Ztry8
Copy link
Copy Markdown

@Ztry8 Ztry8 commented Apr 25, 2026

This PR implements #601: support for number ranges in RON.

It's just syntactic sugar for structures with fields start, end or start, last.
It supports both std::ops and core::range.

Example:

let ranges = RangeTest {
    a: 0..5,
    b: 1..=3,
    c: 0.6..4.3,
    d: 0.3..=5.7,
};

let ser = ron::to_string(&ranges).unwrap();
assert_eq!(ser, "(a:0..5,b:1..=3,c:0.6..4.3,d:0.3..=5.7)");

let de: RangeTest = ron::from_str(&ser).unwrap();
assert_eq!(de, ranges);

Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/ser/mod.rs Outdated
Comment thread tests/601_support_for_number_ranges.rs Outdated
@juntyr
Copy link
Copy Markdown
Member

juntyr commented Apr 25, 2026

Thanks @Ztry8 for prototyping this feature!

@juntyr
Copy link
Copy Markdown
Member

juntyr commented Apr 25, 2026

Also, this addition will need to be documented in the grammar

@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented Apr 26, 2026

Thank you! I've already started working about this

@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented Apr 26, 2026

@juntyr, is everything fine now? What should I put into CHANGELOG.md? Is the grammar.md correct?

Copy link
Copy Markdown
Member

@juntyr juntyr left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, I left some further comments

Comment thread README.md Outdated
Comment thread docs/grammar.md Outdated
Comment thread docs/grammar.md
Comment thread docs/grammar.md Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs
Comment thread src/de/mod.rs
Comment thread src/ser/mod.rs Outdated
Comment thread tests/601_support_for_number_ranges.rs
Comment thread tests/601_support_for_number_ranges.rs
@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented Apr 28, 2026

Thank you! l've already started to working on this

Ztry8 and others added 7 commits April 28, 2026 04:18
Co-authored-by: Juniper Tyree <50025784+juntyr@users.noreply.github.com>
Co-authored-by: Juniper Tyree <50025784+juntyr@users.noreply.github.com>
Co-authored-by: Juniper Tyree <50025784+juntyr@users.noreply.github.com>
Co-authored-by: Juniper Tyree <50025784+juntyr@users.noreply.github.com>
@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented May 2, 2026

@juntyr, thanks for your help and support!

I’ve implemented all of your suggestions. Could you please check?

Sorry I didn’t commit some of your changes.
I had already pushed similar updates before seeing your comments

@juntyr
Copy link
Copy Markdown
Member

juntyr commented May 3, 2026

It seems like you might need to refactor some imports from std to alloc

Copy link
Copy Markdown
Member

@juntyr juntyr left a comment

Choose a reason for hiding this comment

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

Thank you @Ztry8 for your continued work! I've left some more comments

Comment thread README.md Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/de/mod.rs Outdated
Comment thread src/ser/mod.rs Outdated
Comment thread src/ser/mod.rs Outdated
Comment thread src/ser/mod.rs Outdated
Comment thread src/ser/mod.rs
Comment thread src/ser/mod.rs Outdated
@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented May 4, 2026

Thanks! l've already started to working on this

@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented May 8, 2026

@juntyr, thank you for your cooperation!

I’ve implemented all of your suggestions. What do you think about them?

@juntyr
Copy link
Copy Markdown
Member

juntyr commented May 10, 2026

@Ztry8 Thanks for the progress! I'm a bit busy right now, can you fix CI, then I'll have the time for another full review. Thank you for all of your work!

@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented May 10, 2026

I've already started working on fixing the CI
Take your time, and sorry for rushing you

@juntyr
Copy link
Copy Markdown
Member

juntyr commented May 22, 2026

@Ztry8 It seems we're now getting to the tests for ranges but they are failing

@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented May 28, 2026

Yes, there are some issues that I'm currently fixing.
Sorry for the silence, I had some things to deal with related to my studies

@juntyr
Copy link
Copy Markdown
Member

juntyr commented May 29, 2026

@Ztry8 You shouldn’t add Cargo.lock directly - in CI we have the code that generated the lock file with maximum and minimum dependencies. If there’s a version conflict, you can look at that step to see how to fix up dependencies - often it’s resolved by using the correct version bound in Cargo.toml

@Ztry8
Copy link
Copy Markdown
Author

Ztry8 commented May 30, 2026

@juntyr, finding the root cause was quite tedious

The CI uses a minimal-versions lockfile (Cargo.lock.min) generated with -Z minimal-versions, which resolved serde to 1.0.181

It turned out that RangeFrom/RangeTo deserialization support was added to serde in 1.0.193, so I bumped the minimum version requirement to that

Please let me know if bumping the serde minimum version is a problem
As far as I can tell there is no other way around it,
since the range deserialization relies on behavior introduced in that version

Sorry for the noisy chore commits
There are quite a few of them from my debugging process,
but the actual meaningful change is only the version bump in Cargo.toml.
This should not be an issue if the PR is merged with squash

I've run CI on my fork and everything passes.
Thanks for the feedback!!

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.

2 participants