-
Notifications
You must be signed in to change notification settings - Fork 500
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
fix: include_file should handle proto without package #1002
Changes from 8 commits
3ea9f40
ae360ef
9aea9b7
21056cd
ba9e558
511fdd5
f2bf323
b87c205
376adcb
4c52df9
4ff26e5
755ea0d
56c1a2f
8ea9ec9
7b0ccb2
bfb0896
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -1029,8 +1029,8 @@ impl Config { | |||||||
self.write_includes( | ||||||||
modules.keys().collect(), | ||||||||
&mut file, | ||||||||
0, | ||||||||
if target_is_env { None } else { Some(&target) }, | ||||||||
&file_names, | ||||||||
)?; | ||||||||
file.flush()?; | ||||||||
} | ||||||||
|
@@ -1160,65 +1160,56 @@ impl Config { | |||||||
|
||||||||
fn write_includes( | ||||||||
&self, | ||||||||
mut entries: Vec<&Module>, | ||||||||
outfile: &mut fs::File, | ||||||||
depth: usize, | ||||||||
mut modules: Vec<&Module>, | ||||||||
outfile: &mut impl Write, | ||||||||
basepath: Option<&PathBuf>, | ||||||||
) -> Result<usize> { | ||||||||
let mut written = 0; | ||||||||
entries.sort(); | ||||||||
|
||||||||
while !entries.is_empty() { | ||||||||
let modident = entries[0].part(depth); | ||||||||
let matching: Vec<&Module> = entries | ||||||||
.iter() | ||||||||
.filter(|&v| v.part(depth) == modident) | ||||||||
.copied() | ||||||||
.collect(); | ||||||||
{ | ||||||||
// Will NLL sort this mess out? | ||||||||
let _temp = entries | ||||||||
.drain(..) | ||||||||
.filter(|&v| v.part(depth) != modident) | ||||||||
.collect(); | ||||||||
entries = _temp; | ||||||||
file_names: &HashMap<Module, String>, | ||||||||
) -> Result<()> { | ||||||||
modules.sort(); | ||||||||
|
||||||||
let mut stack = Vec::new(); | ||||||||
|
||||||||
for module in modules { | ||||||||
while !module.components.starts_with(&stack) { | ||||||||
stack.pop(); | ||||||||
self.write_line(outfile, stack.len(), "}")?; | ||||||||
} | ||||||||
self.write_line(outfile, depth, &format!("pub mod {} {{", modident))?; | ||||||||
let subwritten = self.write_includes( | ||||||||
matching | ||||||||
.iter() | ||||||||
.filter(|v| v.len() > depth + 1) | ||||||||
.copied() | ||||||||
.collect(), | ||||||||
outfile, | ||||||||
depth + 1, | ||||||||
basepath, | ||||||||
)?; | ||||||||
written += subwritten; | ||||||||
if subwritten != matching.len() { | ||||||||
let modname = matching[0].to_partial_file_name(..=depth); | ||||||||
if basepath.is_some() { | ||||||||
self.write_line( | ||||||||
outfile, | ||||||||
depth + 1, | ||||||||
&format!("include!(\"{}.rs\");", modname), | ||||||||
)?; | ||||||||
} else { | ||||||||
self.write_line( | ||||||||
outfile, | ||||||||
depth + 1, | ||||||||
&format!("include!(concat!(env!(\"OUT_DIR\"), \"/{}.rs\"));", modname), | ||||||||
)?; | ||||||||
} | ||||||||
written += 1; | ||||||||
while stack.len() < module.components.len() { | ||||||||
self.write_line( | ||||||||
outfile, | ||||||||
stack.len(), | ||||||||
&format!("pub mod {} {{", module.part(stack.len())), | ||||||||
)?; | ||||||||
stack.push(module.part(stack.len()).to_owned()); | ||||||||
} | ||||||||
|
||||||||
let file_name = file_names | ||||||||
.get(module) | ||||||||
.expect("every module should have a filename"); | ||||||||
|
||||||||
if basepath.is_some() { | ||||||||
self.write_line( | ||||||||
outfile, | ||||||||
stack.len(), | ||||||||
&format!("include!(\"{}\");", file_name), | ||||||||
)?; | ||||||||
} else { | ||||||||
self.write_line( | ||||||||
outfile, | ||||||||
stack.len(), | ||||||||
&format!("include!(concat!(env!(\"OUT_DIR\"), \"/{}\"));", file_name), | ||||||||
)?; | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
for depth in (0..stack.len()).rev() { | ||||||||
self.write_line(outfile, depth, "}")?; | ||||||||
} | ||||||||
Ok(written) | ||||||||
|
||||||||
Ok(()) | ||||||||
} | ||||||||
|
||||||||
fn write_line(&self, outfile: &mut fs::File, depth: usize, line: &str) -> Result<()> { | ||||||||
fn write_line(&self, outfile: &mut impl Write, depth: usize, line: &str) -> Result<()> { | ||||||||
outfile.write_all(format!("{}{}\n", (" ").to_owned().repeat(depth), line).as_bytes()) | ||||||||
} | ||||||||
|
||||||||
|
@@ -1833,4 +1824,57 @@ mod tests { | |||||||
f.read_to_string(&mut content).unwrap(); | ||||||||
content | ||||||||
} | ||||||||
|
||||||||
#[test] | ||||||||
fn test_write_includes() { | ||||||||
let modules = [ | ||||||||
Module::from_protobuf_package_name("foo.bar.baz"), | ||||||||
Module::from_protobuf_package_name(""), | ||||||||
Module::from_protobuf_package_name("foo.bar"), | ||||||||
Module::from_protobuf_package_name("bar"), | ||||||||
Module::from_protobuf_package_name("foo"), | ||||||||
Module::from_protobuf_package_name("foo.bar.qux"), | ||||||||
Module::from_protobuf_package_name("foo.bar.a.b.c"), | ||||||||
]; | ||||||||
|
||||||||
let file_names = modules | ||||||||
.iter() | ||||||||
.map(|m| (m.clone(), m.to_file_name_or("_.default"))) | ||||||||
.collect(); | ||||||||
|
||||||||
let mut buf = Vec::new(); | ||||||||
Config::new() | ||||||||
.default_package_filename("_.default") | ||||||||
.write_includes(modules.iter().collect(), &mut buf, None, &file_names) | ||||||||
.unwrap(); | ||||||||
let expected = [ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other tests read the expected result from a file. I think that improves maintainalibity. For example: Lines 1682 to 1684 in 04be604
|
||||||||
r#"include!(concat!(env!("OUT_DIR"), "/_.default.rs"));"#, | ||||||||
r#"pub mod bar {"#, | ||||||||
r#" include!(concat!(env!("OUT_DIR"), "/bar.rs"));"#, | ||||||||
r#"}"#, | ||||||||
r#"pub mod foo {"#, | ||||||||
r#" include!(concat!(env!("OUT_DIR"), "/foo.rs"));"#, | ||||||||
r#" pub mod bar {"#, | ||||||||
r#" include!(concat!(env!("OUT_DIR"), "/foo.bar.rs"));"#, | ||||||||
r#" pub mod a {"#, | ||||||||
r#" pub mod b {"#, | ||||||||
r#" pub mod c {"#, | ||||||||
r#" include!(concat!(env!("OUT_DIR"), "/foo.bar.a.b.c.rs"));"#, | ||||||||
r#" }"#, | ||||||||
r#" }"#, | ||||||||
r#" }"#, | ||||||||
r#" pub mod baz {"#, | ||||||||
r#" include!(concat!(env!("OUT_DIR"), "/foo.bar.baz.rs"));"#, | ||||||||
r#" }"#, | ||||||||
r#" pub mod qux {"#, | ||||||||
r#" include!(concat!(env!("OUT_DIR"), "/foo.bar.qux.rs"));"#, | ||||||||
r#" }"#, | ||||||||
r#" }"#, | ||||||||
r#"}"#, | ||||||||
r#""#, | ||||||||
] | ||||||||
.join("\n"); | ||||||||
let actual = String::from_utf8(buf).unwrap(); | ||||||||
assert_eq!(expected, actual); | ||||||||
} | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
use alloc::{string::ToString, vec}; | ||
|
||
use self::proto::image::Image; | ||
use self::proto::post::content::post_content_fragment; | ||
use self::proto::post::content::PostContentFragment; | ||
use self::proto::post::Post; | ||
use self::proto::user::User; | ||
use self::proto::Timestamp; | ||
|
||
mod proto { | ||
include!(concat!(env!("OUT_DIR"), "/complex_package_structure/__.rs")); | ||
} | ||
|
||
#[test] | ||
fn test_complex_package_structure() { | ||
let user = User { | ||
id: "69a4cd96-b956-4fb1-9a97-b222eac33b8a".to_string(), | ||
name: "Test User".to_string(), | ||
created_at: Some(Timestamp { | ||
seconds: 1710366135, | ||
nanos: 0, | ||
}), | ||
..User::default() | ||
}; | ||
let posts = vec![ | ||
Post::default(), | ||
Post { | ||
id: "aa1e751f-e287-4c6e-aa0f-f838f96a1a60".to_string(), | ||
author: Some(user), | ||
content: vec![ | ||
PostContentFragment { | ||
content: Some(post_content_fragment::Content::Text( | ||
"Hello, world!".to_string(), | ||
)), | ||
}, | ||
PostContentFragment { | ||
content: Some(post_content_fragment::Content::Image(Image { | ||
name: "doggo.jpg".to_string(), | ||
description: Some("A dog".to_string()), | ||
data: vec![0, 1, 2, 3], | ||
})), | ||
}, | ||
PostContentFragment { | ||
content: Some(post_content_fragment::Content::Link( | ||
"https://example.com".to_string(), | ||
)), | ||
}, | ||
], | ||
..Post::default() | ||
}, | ||
Post::default(), | ||
]; | ||
assert_eq!(posts.len(), 3); | ||
assert_eq!(posts[1].content.len(), 3); | ||
if let PostContentFragment { | ||
content: Some(post_content_fragment::Content::Image(Image { name, .. })), | ||
} = &posts[1].content[1] | ||
{ | ||
assert_eq!(name, "doggo.jpg"); | ||
} else { | ||
assert!(false, "Expected an image") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[*] | ||
ij_protobuf_keep_blank_lines_in_code = 0 | ||
insert_final_newline = true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
syntax = "proto3"; | ||
|
||
package post.content; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this complex example testing?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This example has multiple files, some with packages, and some without. It also contains nested packages, and imports between everything. I wanted to make sure that the generated include file is correct for a combination of these edge cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My intention is not to remove the test. All functionality should have a test. You solve a bug, so the bugged behavior should be tested. My comment is in two parts:
|
||
|
||
import "wrong/image.proto"; | ||
|
||
message PostContentFragment { | ||
oneof content { | ||
string text = 1; | ||
image.Image image = 2; | ||
string link = 3; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
syntax = "proto3"; | ||
|
||
package post; | ||
|
||
import "std/time.proto"; | ||
import "user/user.proto"; | ||
import "post/content.proto"; | ||
|
||
message Post { | ||
string id = 1; | ||
string title = 2; | ||
reserved 3; | ||
user.User author = 4; | ||
Timestamp created_at = 5; | ||
Timestamp updated_at = 6; | ||
repeated content.PostContentFragment content = 7; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
syntax = "proto3"; | ||
|
||
message Timestamp { | ||
int64 seconds = 1; | ||
int32 nanos = 2; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
syntax = "proto3"; | ||
|
||
package user; | ||
|
||
import "std/time.proto"; | ||
|
||
message User { | ||
string id = 1; | ||
Timestamp created_at = 2; | ||
Timestamp updated_at = 3; | ||
string name = 4; | ||
string email = 5; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// This file makes sure that the file path of the proto file has nothing to do with the package name, | ||
// therefore testing the validity of write_includes. | ||
|
||
syntax = "proto3"; | ||
|
||
package image; | ||
|
||
message Image { | ||
string name = 1; | ||
optional string description = 2; | ||
bytes data = 3; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
syntax = "proto3"; | ||
|
||
message NoPackageWithMessageExampleMsg { | ||
string some_field = 1; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this test already exists: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I meant is: the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
mod proto { | ||
include!(concat!(env!("OUT_DIR"), "/no_package/_includes.rs")); | ||
} | ||
|
||
#[test] | ||
fn it_works() { | ||
assert_eq!( | ||
proto::NoPackageWithMessageExampleMsg::default(), | ||
proto::NoPackageWithMessageExampleMsg::default() | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this test. It is simple and to the point. And it doesn't require
build.rs
.