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

Added serde1 feature to Serialize/Deserialize WeightedIndex #974

Merged
merged 14 commits into from May 18, 2020

Conversation

CGMossa
Copy link
Contributor

@CGMossa CGMossa commented May 1, 2020

It required adding serde as an optional dependency.
Also added a test using bincode like in the other crate.

Referering to this: #967

I hope this is up to standard, otherwise I'll take all of your feedback and ensure it is as correct
as possible.

It required adding serde as an optional dependency.
Also added a test using bincode like in the other crate.
@dhardy
Copy link
Member

dhardy commented May 1, 2020

Thanks @CGMossa. Hmm, would it be possible to do the reformatting first in its own commit? Also please add a note to the changelog.

https://rust-random.github.io/book/contributing.html

@CGMossa
Copy link
Contributor Author

CGMossa commented May 1, 2020 via email

@dhardy
Copy link
Member

dhardy commented May 1, 2020

I thought you'd suggest more things to add the derive attribute to. I'd be
happy to add it everywhere needed. But I don't know which places.

It crossed my mind that this might be a good idea, but I didn't look into where yet. Probably would only be applicable to distributions.

@CGMossa
Copy link
Contributor Author

CGMossa commented May 2, 2020 via email

@vks
Copy link
Collaborator

vks commented May 2, 2020

It looks like the minimal supported serde version is incorrectly specified.

@CGMossa
Copy link
Contributor Author

CGMossa commented May 3, 2020

I wrote it exactly like it is written in rand_core and rand_pcg. Which version should I write instead?
Should I use this

serde = "1.0.106"

or

serde = "1.0.*"

?

Hopefully this will do it.
@CGMossa
Copy link
Contributor Author

CGMossa commented May 3, 2020

I see that tha CI stuff is resulting in an error. I am unable to see the same error here. I've added a version number to one of the Cargo.toml and I'll wait and see. Even if I run the command that is making the error cargo test --features=serde1,log I don't get an error. But I have serde 1.0.105 installed and the latest is 1.0.106 and maybe the thing on the here has an earlier version?

@CGMossa
Copy link
Contributor Author

CGMossa commented May 3, 2020

This worked. I've no idea why, since the other crates in the rand-workspace also indicated serde with "1". Maybe they fixed a bug? Although, not this works.

@dhardy
Copy link
Member

dhardy commented May 4, 2020

We test with the minimal version of Serde which crates say they support — but this must be unified across all dependencies, so will be the highest minimum version. The idea is to find the real minimum version, but I think most people only care that it's high enough to work. That said, 1.0.105 is less than two months old. If an older version suffices that would be preferable. You can test locally via cargo generate-lockfile -Z minimal-versions

@dhardy
Copy link
Member

dhardy commented May 4, 2020

For completeness's sake (if you don't mind), we should also consider: Bernoulli, Uniform, UniformFloat, UniformDuration, StepRng, IndexVec (maybe).
The following structs have zero size (thus don't need serialising), but doing so might ease deriving of other types: OpenClosed01, Open01, Standard, Alphanumeric.

@CGMossa
Copy link
Contributor Author

CGMossa commented May 4, 2020 via email

@CGMossa
Copy link
Contributor Author

CGMossa commented May 4, 2020

Minimal supported serde version is 1.0.103. Release note says:

Support deserializing untagged unit variants from formats that treat unit as None (#1668)

Test command:

rustup run nightly cargo generate-lockfile -Z minimal-versions & cargo test --features=serde1,log

#TODO: Test of equality between ser/de is needed.
#TODO: Unable to test equality in mode between de-serialized and original UniformDuration
@dhardy
Copy link
Member

dhardy commented May 4, 2020

1.0.103

This version is almost six months old. Since we're developing against the master branch (will be a breaking release), this is fine IMO.

@CGMossa
Copy link
Contributor Author

CGMossa commented May 4, 2020

I've added the serialize and deserialize annotations. I also wanted to add tests using bincode, but some of these types have no fields I can test equality against, and PartialEq is not implemented for them so.. I don't know.

I've left some skeleton code, awaiting further feedback.

(this is like my biggest attempt at contributing yet! Thanks for the PR-mentoring!!)

@CGMossa
Copy link
Contributor Author

CGMossa commented May 14, 2020

Am I missing more things here?

@dhardy
Copy link
Member

dhardy commented May 14, 2020

Sorry, I'll try to review in the next couple of days.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Sorry, I haven't been paying this project much attention lately!

Your PR looks good. Those unfinished bits shouldn't be hard (I gave a couple hints).

src/seq/index.rs Outdated Show resolved Hide resolved
src/distributions/uniform.rs Outdated Show resolved Hide resolved
@CGMossa
Copy link
Contributor Author

CGMossa commented May 15, 2020

I've added the tests based on your suggestions.

Do I resolve them or do I wait for you to click that they are resolved?

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Looks good to me. (There are some over-long lines, and you didn't need to match cases which can't occur in the tests, but it's still good enough.)

I'll wait a couple of days in case @vks wishes to review, then merge.

Cargo.toml Show resolved Hide resolved
src/seq/index.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

Looks good!

@vks
Copy link
Collaborator

vks commented May 15, 2020

This should probably be squashed before merging. I think this is technically a breaking change.

@CGMossa
Copy link
Contributor Author

CGMossa commented May 15, 2020

I don't know how to squash?

@vks
Copy link
Collaborator

vks commented May 15, 2020

I think GitHub offers this option to the maintainers, but it seems like it is disabled for this repository.

Edit: I just enabled it, you don't have to do anything, we can let GitHub do it.

If you want to do that yourself, you can use git rebase -i to specify which commits should be squashed.

@vks vks changed the title Added serde1 feature to Serialize/Deseriaslize WeightedIndex Added serde1 feature to Serialize/Deserialize WeightedIndex May 15, 2020
@dhardy
Copy link
Member

dhardy commented May 16, 2020

master already contains breaking changes, so no problem there.

@CGMossa
Copy link
Contributor Author

CGMossa commented May 17, 2020

I've added the missing spaces. Thanks for enabling that feature, as I've got no training in using rebase or squashing. I'd appreciate a link to learn about this.

@dhardy
Copy link
Member

dhardy commented May 18, 2020

Thanks for the PR!

git rebase is quite useful. Also git add -p. You may want to test them out on a dummy repo!

@dhardy dhardy merged commit 7ede440 into rust-random:master May 18, 2020
kazcw added a commit to kazcw/rand that referenced this pull request May 19, 2020
Implements rust-random#974. As in rust-random#975, but defining equality such that the user is
not exposed to the fact that one logical state may have different
representations in an implementation-specific way.
kazcw added a commit to kazcw/rand that referenced this pull request May 28, 2020
Implements rust-random#974. As in rust-random#975, but defining equality such that the user is
not exposed to the fact that one logical state may have different
representations in an implementation-specific way.
dhardy pushed a commit that referenced this pull request May 29, 2020
Implements #974. As in #975, but defining equality such that the user is
not exposed to the fact that one logical state may have different
representations in an implementation-specific way.
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