Testkit to protobuf #1078
Testkit to protobuf #1078
Conversation
dvush
requested review from
alekseysidorov,
aleksuss and
slowli
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1078 +/- ##
==========================================
+ Coverage 92.91% 92.93% +0.01%
==========================================
Files 162 162
Lines 24242 24259 +17
==========================================
+ Hits 22525 22544 +19
+ Misses 1717 1715 -2
Continue to review full report at Codecov.
|
| &["examples/timestamping/proto"], | ||
| "timestamping_example_protobuf_mod.rs", | ||
| ); | ||
| } |
There was a problem hiding this comment.
What do you think about to change this repeated function calls for loop ?
For something like this
extern crate exonum_build;
use exonum_build::protobuf_generate;
fn main() {
let exonum_protos = exonum_build::get_exonum_protobuf_files_path();
let proto_data = [
("src/proto", vec!["src/proto"], "testkit_protobuf_mod.rs"),
(
"tests/inflating_currency/proto",
vec!["tests/inflating_currency/proto", &exonum_protos],
"currency_example_protobuf_mod.rs",
),
(
"tests/counter/proto",
vec!["tests/counter/proto"],
"counter_example_protobuf_mod.rs",
),
(
"tests/service_hooks/proto",
vec!["tests/service_hooks/proto"],
"hooks_example_protobuf_mod.rs",
),
(
"examples/timestamping/proto",
vec!["examples/timestamping/proto"],
"timestamping_example_protobuf_mod.rs",
),
];
for (proto_path, includes, mod_file_name) in proto_data.iter() {
protobuf_generate(proto_path, includes, mod_file_name);
}
}There was a problem hiding this comment.
Or even to create some structure in exonum_build crate for holding this data:
struct ProtoData {
proto_path: String,
includes: Vec<String>,
mod_file_name: String
}There was a problem hiding this comment.
I think they are equivalent because there are no reduction in the code size.
To me current version looks more simple.
There was a problem hiding this comment.
I'd support @dvush in that repeated function calls and repeated tuples/structures with a loop (or map) have the same readability. We have to repeat either function calls or struct/tuple constructors so it does not make any difference here, so I'm personally fine with function calls. Verbosity here comes from essential complexity of having 4 sets of 3 distinct paths that are not going anywhere.
There may be other considerations for choosing structs (and not tuples) because with something like this
Proto {
proto_dir: "tests/service_hooks/proto",
include_dirs: vec!["tests/service_hooks/proto"],
rust_module: "hooks_example_protobuf_mod.rs",
}it's easier to see what path stands for what, when compared to
(
"tests/service_hooks/proto",
vec!["tests/service_hooks/proto"],
"hooks_example_protobuf_mod.rs",
)Such 'named arguments' may actually improve readability.
|
|
||
| // For protobuf generated files. | ||
| #![allow(bare_trait_objects)] | ||
| #![allow(renamed_and_removed_lints)] |
There was a problem hiding this comment.
Is it possible to teach rust-protobuf print this on its own? I'm not a fan of boilerplate that we have to keep forever in out files because someone else does something stupid. rust-protobuf seems to output quite a few clippy annotations, why don't we ask them nicely to add some more?
There was a problem hiding this comment.
Unfortunately no, it is known issue. Maybe it is even fixed, but not released yet.
There was a problem hiding this comment.
I think that we can patch generated protobuf file as workaround and later we should push fixes to the upstream protobuf repo.
There was a problem hiding this comment.
Why not just use current workaround without patching generated files? It seems simpler.
Unfortunately we cannot add this annotations to file which we import! for the same reason this workaround with import!ing piece of the mod file exists.
There was a problem hiding this comment.
I don't think we should waste our limited resources, so current solution is preferable.
|
|
||
| //! Module of the rust-protobuf generated files. | ||
|
|
||
| // For protobuf generated files. |
There was a problem hiding this comment.
Could we add a few words about what it does for protobuf generated files? The meaning of this sentence is a bit unclear to me.
There was a problem hiding this comment.
This is comment attributed to next two lines - which are attributes that allow some bad style in code generated files.
I think since it is inner doc it is informative enough.
Overview
Definition of Done