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

Address backwards-incompatible clippy warnings in autogenerated code #113

Merged

Conversation

ysimonson
Copy link
Contributor

In conjunction with #112, this addresses all of the clippy warnings I'm seeing for capnpc autogenerated code in my schema, by fixing for https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#wrong_self_convention

However, this is a breaking change, so it's in a separate PR. Calling code will have to change instances of as_reader to into_reader, and from_server to into_client.

@ysimonson
Copy link
Contributor Author

ysimonson commented Nov 8, 2018

Note: this is not ready to merge yet. I'm seeing a lot of references to as_reader and from_server elsewhere in the codebase, and was hoping to get:

  1. A thumbs up/down on whether this PR makes sense.
  2. How I should proceed on addressing those references. Tests are already passing locally, but do I need to call a command to re-generate some code, e.g. for schema_capnp.rs? Do I just need to find/replace all instances? Something else...?

@dwrensha
Copy link
Member

dwrensha commented Nov 8, 2018

I think this change makes sense. I've been reluctant to diverge from the names used in the analogous capnproto-c++ methods, but making the code idiomatic for Rust is probably more important than that concern.

The smoothest way to make the change would be to keep the old methods around and add deprecation annotations on them. Then people could gradually update. We would delete the old methods on the next version bump.

@ysimonson
Copy link
Contributor Author

OK, I added the methods back with deprecation notices. I've also updated the references to the old methods in tests and the readme.

@dwrensha
Copy link
Member

dwrensha commented Nov 8, 2018

Hm... we also need to maintain consistency with the methods on the built-in types, of which there are many. For example, text_list::Builder::as_reader() would need to be deprecated in favor of text_list::Builder::into_reader().

@ysimonson
Copy link
Contributor Author

ysimonson commented Nov 8, 2018

That makes sense. I got all the methods I could find. I haven't re-generated capnp/fuzz/fuzzers/test_capnp.rs and capnpc/src/schema_capnp.rs - how would I go about doing that?

@dwrensha
Copy link
Member

Thanks!

@dwrensha
Copy link
Member

added some cleanup in e200353

vhdirk pushed a commit to vhdirk/capnproto-rust that referenced this pull request May 8, 2019
…ppy-warnings

Address backwards-incompatible clippy warnings in autogenerated code
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.

2 participants