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

backwards compatibility issue in the last month or so #3721

Closed
nrc opened this issue Aug 2, 2019 · 2 comments
Closed

backwards compatibility issue in the last month or so #3721

nrc opened this issue Aug 2, 2019 · 2 comments
Labels
bug Panic, non-idempotency, invalid code, etc.

Comments

@nrc
Copy link
Member

nrc commented Aug 2, 2019

Some time between 2019-06-14 and 2019-07-19, the default formatting changed, this breaks rustfmt's backwards compatibility guarantee.

I see two kinds of changes:

-            ConfChangeType::BeginMembershipChange | ConfChangeType::FinalizeMembershipChange => {
-                unimplemented!()
-            }
+            ConfChangeType::BeginMembershipChange | ConfChangeType::FinalizeMembershipChange => unimplemented!(),

and

-                Datum::Bytes(
-                    "НОЧЬ НА ОКРАИНЕ МОСКВЫ"
-                        .as_bytes()
-                        .to_vec(),
-                ),
+                Datum::Bytes("НОЧЬ НА ОКРАИНЕ МОСКВЫ".as_bytes().to_vec()),
+                Datum::Bytes("НОЧЬ НА ОКРАИНЕ МОСКВЫ".as_bytes().to_vec()),

The second seems to require >1 byte characters.

There are many more instances in https://github.com/tikv/tikv/pull/5137/files

Thanks!

@nrc nrc added the bug Panic, non-idempotency, invalid code, etc. label Aug 2, 2019
@scampi
Copy link
Contributor

scampi commented Aug 5, 2019

Below is the list of commits for the range reported by @nrc. Few are mine and probably the second change (about unicode) is probably mine where I didn't gated the changes. I will have a look at this one.

@nrc
Copy link
Member Author

nrc commented Aug 5, 2019

Thanks @scampi ! Fwiw, I think the breaking change for unicode might be acceptable? If it's a one-off and it is communicated. It is clearly a big fix and shouldn't affect too many users. OTOH, having an opt-in also seems good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

No branches or pull requests

3 participants