Skip to content

Conversation

vks
Copy link
Collaborator

@vks vks commented Sep 15, 2021

This fixes some clippy warnings (due to new lints on nightly).

@dhardy The changes will break serialization compatibility with older versions. Do we have a policy whether that is a breaking or value-breaking change?

@dhardy
Copy link
Member

dhardy commented Sep 16, 2021

I don't think we do have a policy there. It could be considered API-breaking since older serialised values will not deserialise.

@dhardy dhardy added the B-API Breakage: API label Sep 16, 2021
@newpavlov
Copy link
Member

I also think it should be considered a breaking change (this is one of the reasons why I am generally conservative regarding implementation of the serde traits).

@vks
Copy link
Collaborator Author

vks commented Sep 16, 2021

So we have to treat fields of serializable struct as public? Personally, I would rather treat it like value stability. Serializing and storing the seed are similar use cases.

@dhardy
Copy link
Member

dhardy commented Sep 17, 2021

What is the difference between "value-breaking" and "API-breaking" in this case? Both imply it cannot be merged into "0.8.x". In regards to post-1.0 minor releases, they are one of those weird things that the packaging system effectively treats as a breaking change but semver spec says should not be. In that case I'd be inclined to say we should decide on a case-by-case basis based on how likely the change is to be noticed — but we can talk about this later.

@vks
Copy link
Collaborator Author

vks commented Sep 17, 2021

What is the difference between "value-breaking" and "API-breaking" in this case? Both imply it cannot be merged into "0.8.x".

That's fair, but this applies to the distinction between value-breaking and API-breaking in general, no? Before 1.0, our distinction between value-breaking and API-breaking does not make a difference for versioning.

@dhardy
Copy link
Member

dhardy commented Sep 17, 2021

Well... I see no other reason you would care about the distinction?

@vks
Copy link
Collaborator Author

vks commented Sep 17, 2021

If we decide on a policy for serialization stability now, it will affect future releases (including post-1.0), where it does make a difference.

@vks vks mentioned this pull request Oct 21, 2021
@vks
Copy link
Collaborator Author

vks commented Feb 22, 2022

This still needs an update to the changelog.

@dhardy
Copy link
Member

dhardy commented Feb 22, 2022

Agreed (your PR).

This targets rand_distr (other than a Clippy fix); I think we can land breaking changes there too.

This breaks serialization compatibility with older versions.
@vks vks force-pushed the remove-unused-fields branch from f5d1db1 to 5f0d3a1 Compare March 29, 2022 22:56
@vks
Copy link
Collaborator Author

vks commented Mar 29, 2022

@dhardy I finally updated the changelog, could you please have a look? Thanks!

@vks vks added the D-review Do: needs review label Mar 29, 2022
@vks vks merged commit f0f15b5 into rust-random:master Apr 21, 2022
@vks vks deleted the remove-unused-fields branch April 21, 2022 16:54
benjamin-lieser pushed a commit to benjamin-lieser/rand that referenced this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API D-review Do: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants