Rustfmt and clippy ignores, test harness updates

tafia / quick-protobuf

A rust implementation of protobuf parser

MIT License

452 stars 87 forks source link

Rustfmt and clippy ignores, test harness updates #51

Closed koivunej closed 7 years ago

koivunej commented 7 years ago

This adds relevant clippy and rustfmt ignores to generated output. There is one problem though:

$ cargo test
   Compiling quick-protobuf v0.4.0 (file:///home/joonas/src/quick-protobuf)
error: expected item after attributes
 --> tests/rust_protobuf/v2/test_group_pb.rs:8:34
  |
8 | #[cfg_attr(rustfmt, rustfmt_skip)]
  |                                  ^
error: aborting due to previous error

It errors out because tests/rust_protobuf/v2/test_group_pb.proto does not produce any messages as in the generated file contains only attributes, and apparently cfg_attr cannot be present without any rust code. Could this be a missed test case or something?

tafia commented 7 years ago

Well, groups are not supported so indeed there is no rust code (and no tests). We should probably check if there is something to write first.

I don't plan to add group support anytime soon because it has been deprecated and I prefer spending time elsewhere. but if you (or anyone reading this) want it I welcome any PR.

koivunej commented 7 years ago

I don't plan to add group support anytime soon because it has been deprecated and I prefer spending time elsewhere. but if you (or anyone reading this) want it I welcome any PR.

I have no need for this. I guess I could just remove the problematic file then, perhaps add an error for unsupported feature?

Now that I'm back at my original project, looks like we probably want to add #![allow(dead_code)], otherwise every messages from_reader will trigger a warning.

koivunej commented 7 years ago

My latest push handles that problematic codegen file as "should fail". In the CI report there should now be the following files as failing the codegen:

../tests/rust_protobuf/v2/test_root_pb.proto
../tests/rust_protobuf/v2/test_group_pb.proto EDIT: fixed
../tests/rust_protobuf/v3/test_enum_alias_pb.proto
../tests/rust_protobuf/v2/test_expose_oneof_pb.proto
../tests/rust_protobuf/v2/test_enum_alias_pb.proto

Can you comment on the rest of the files, should they codegen ok?

tafia commented 7 years ago

This is very nice thanks. I am well less versed in shell scripts than you 😄

I have 2 remarks:

we could extend the list of must fail
- ../tests/rust_protobuf/v2/test_root_pb.proto : root search is not implemented yet
- ../tests/rust_protobuf/v3/test_enum_alias_pb.proto: enum alias not implemented
- ../tests/rust_protobuf/v2/test_enum_alias_pb.proto: same
- ../tests/rust_protobuf/v2/test_expose_oneof_pb.proto: this one is a known issue (there should be some missing file in rust-protobuf repo, I didn't spend time figuring this out yet)
For the issue with #[cfg_attr(rustfmt, rustfmt_skip)] on empty file: we should probably delete the generated file if there is an error ... or rename it to something like file.rs.broken ? (and comment the mod test_group_pb line anyway as it doesn't do anything

koivunej commented 7 years ago

With the latest push I've:

added comments around the bash script
added the failures you described
removed the non-generating .rs files and unlisted the modules
fixed the cfg_attr typo (!!!)

As the CI runs now complete I think WIP can be removed and this thing is ready to be merged.

I am well less versed in shell scripts than you :smile:

Not sure if it's a good thing :) Perhaps one day someone comes along and replaces the shell script with more readable rust code but I think the beefed up version will do for some time. One critical issue I fixed in the shell scripts is that they ignored all failures that did not happen on the last executed command (as sh/bash does without set -e).

tafia commented 7 years ago

Merging!

tafia commented 7 years ago

Thanks for the details. Don't worry it's fine!