-
Notifications
You must be signed in to change notification settings - Fork 17
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
Support splitting a single item into multiple files #270
base: main
Are you sure you want to change the base?
Conversation
I've only added a single test so far, considering this is a proposal and not a final solution. |
e2e114b
to
4c26560
Compare
4c26560
to
42e6e05
Compare
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.
Thanks very much for this contribution!
I think we should add the type prefix to those generated files, since different types may not be in the same resolve namespace. For example, we can have a service A
and struct A
at the same time.
LGTM. Maybe a test for the workspace mode can be added? |
@Millione Hi. I tried adding a test for a workspace, but couldn't find any existing tests for it. And my test fails with a panic at crate::Builder::thrift()
.ignore_unused(false)
.compile_with_config(
vec![IdlService::from_path(source.to_owned())],
crate::Output::Workspace(target.into()),
) Could you please point me to an existing test for a workspace mode, or help me figure out why |
Unfortunately, there is no existing test for this, but I'll do my best to help you figure it out.
The difference between normal mode and workspace mode is that in workspace mode the code generation doesn't take place in the build phase, but in the run phase. It generates multiple crates and adds all these crates as workspace members to the Cargo.toml file.
In my opinion, we should put the workspace mode test in a separate tests folder, so that it is a package with a binary target and fits into the real usage. |
I added tests for the workspace mode, as well as splitting the files in the workspace mode. |
d0a79d4
to
bcccf42
Compare
Fantastic jobs!
Maybe that's a bit too much? I think one thrift test is enough, but the tested thrift files should be well-constructed for the workspace, e.g. A.thrift and B.thrift both use C.thrift struct, and see if the struct is generated in common crates and used correctly by A and B. |
Maybe more complex? for example, more layers of nesting mods/namespaces? |
Could you maybe write down the |
Sorry for the late reply, I've constructed five idl files. article.thrift: include "image.thrift"
include "author.thrift"
include "common.thrift"
namespace rs article
enum Status {
NORMAL = 0,
DELETED = 1,
}
struct Article {
1: required i64 id,
2: required string title,
3: required string content,
4: required author.Author author,
5: required Status status,
6: required list<image.Image> images,
7: required common.CommonData common_data,
}
struct GetArticleRequest {
1: required i64 id,
}
struct GetArticleResponse {
1: required Article article,
}
service ArticleService {
GetArticleResponse GetArticle(1: GetArticleRequest req),
} author.thrift: include "image.thrift"
include "common.thrift"
namespace rs author
struct Author {
1: required i64 id,
2: required string username,
3: required string email,
4: required image.Image avatar,
5: required common.CommonData common_data,
}
struct GetAuthorRequest {
1: required i64 id,
}
struct GetAuthorResponse {
1: required Author author,
}
service AuthorService {
GetAuthorResponse GetAuthor(1: GetAuthorRequest req),
} cdn.thrift: include "common.thrift"
namespace rs article.image.cdn
struct CDN {
1: required i64 id,
2: required string url,
3: required common.CommonData common_data,
} common.thrift: namespace rs common
struct CommonData {
1: required i64 id,
2: required string name,
3: required string description,
} image.thrift: include "common.thrift"
include "cdn.thrift"
namespace rs article.image
struct Image {
1: required i64 id,
2: required string url,
3: required cdn.CDN cdn,
4: required common.CommonData common_data,
}
struct GetImageRequest {
1: required i64 id,
}
struct GetImageResponse {
1: required Image image,
}
service ImageService {
GetImageResponse GetImage(1: GetImageRequest req),
} And there are three services that can be used in workspace mode, in |
bcccf42
to
c6682f0
Compare
@PureWhiteWu Thanks, I've replaced the workspace tests with the one you provided, it seems to work fine. |
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.
It seems not right?
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.
It seems that the ImageService
trait not generated?
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.
If article.thrift has its own Author
struct, will the filename conflict?
Motivation
To allow generating code for a single entity in multiple files. See a related issue in
volo
- cloudwego/volo#454Solution
Introduce a new
split
mode that can be enabled using the builder