Skip to content

Conversation

@allengeorge
Copy link
Contributor

Client: rs

@allengeorge allengeorge force-pushed the thrift-4529 branch 2 times, most recently from 06d7b94 to d1a1bcb Compare November 10, 2018 18:02
@allengeorge
Copy link
Contributor Author

Build failure caused by a PEBCAK on my part (forgot to change enum variant names in cross-tests). Fixing now.

@jeking3
Copy link
Contributor

jeking3 commented Nov 11, 2018

I assume the default setting is to use the previous way, and this is a new way, so it is backwards compatible?

@allengeorge allengeorge changed the title THRIFT-4529: Camel-case enum variants THRIFT-4529: Camel-case Rust enum variants Nov 11, 2018
@allengeorge
Copy link
Contributor Author

I did not include a flag for backwards-compatibility. I'd prefer not to, because this change makes the autogenerated code conform to Rust naming conventions.

Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

This is a breaking change based on the conversation. That means it needs a lib/rs/README.md entry indicating such, and an entry in the top level CHANGES file in the appropriate section for breaking changes.

@allengeorge
Copy link
Contributor Author

Yes - you're right: it is a breaking change. I'll make the README entries, and make sure I do so for any (hopefully rare!) breaking changes I make in the future. Thank you for catching this.

@allengeorge allengeorge changed the title THRIFT-4529: Camel-case Rust enum variants THRIFT-4529: Rust enum variants are now camel-cased Nov 11, 2018
@allengeorge
Copy link
Contributor Author

allengeorge commented Nov 11, 2018

I've updated the CHANGES file, plus added a Compatibility section to the Rust README along with transition guidelines. I will do this in the future for any breaking changes.

@allengeorge
Copy link
Contributor Author

@jeking3 I've made the requested changes, and, the only thing that fails in Travis is the SBCL build under autotools :/ Could this be merged please?

@jeking3 jeking3 merged commit b57d126 into apache:master Nov 12, 2018
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.

3 participants