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

Remove unused fields #1184

Merged
merged 1 commit into from
Apr 21, 2022
Merged

Remove unused fields #1184

merged 1 commit into from
Apr 21, 2022

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
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