chore(prost_build): introduce a typeset FullyQualifiedName #1191
Conversation
Removes the need for multiple runtime variant assertions.
| self.extern_paths | ||
| .iter() | ||
| .map(|(a, b)| (a.as_str(), b.as_str())), | ||
| self.prost_types, |
There was a problem hiding this comment.
To be honest, if the goal is to simplify, then that failed here. The previous version was simpler.
I think Config::extern_paths should change type to better match the internals of ExternPaths. If extern_paths is a HashMap, then it can be passed to ExternPaths::new and used without doing a manual copy.
| proto_path: impl Into<String>, | ||
| rust_path: impl Into<String>, |
There was a problem hiding this comment.
Thanks, this is a great improvement.
| @@ -0,0 +1,52 @@ | |||
| use itertools::Itertools; | |||
|
|
|||
| // Invariant: should always begin with a '.' (dot) | |||
There was a problem hiding this comment.
| // Invariant: should always begin with a '.' (dot) | |
| /// Represents a fully-qualified name of the message type or field, scope delimited by periods. | |
| /// | |
| /// For example, message type "Foo" which is declared in package "bar" has fully qualified name | |
| /// ".bar.Foo". If that type has a field "Baz", Baz's full name is ".bar.Foo.Baz". | |
| /// | |
| /// This value must always begin with a '.' (dot) |
| @@ -101,7 +106,8 @@ impl<T> std::iter::FusedIterator for Iter<'_, T> {} | |||
| /// - the global path | |||
| /// | |||
| /// Example: sub_path_iter(".a.b.c") -> [".a.b.c", "a.b.c", "b.c", "c", ".a.b", ".a", "."] | |||
| fn sub_path_iter(full_path: &str) -> impl Iterator<Item = &str> { | |||
| fn sub_path_iter(full_path: &FullyQualifiedName) -> impl Iterator<Item = &str> { | |||
| let full_path = full_path.as_ref(); | |||
There was a problem hiding this comment.
To me, it makes more sense to have a as_str function. Using as_ref is clever, but it took me a while to discover why you are doing it.
| mod test_helpers { | ||
| use super::*; | ||
|
|
||
| impl From<&str> for FullyQualifiedName { |
There was a problem hiding this comment.
Why is this behind #[cfg(test)]?
| self.path.push(8); | ||
| self.path.push(oneof.path_index); | ||
| self.append_doc(fq_message_name, None); | ||
| self.path.pop(); | ||
| self.path.pop(); | ||
|
|
||
| let oneof_name = format!("{}.{}", fq_message_name, oneof.descriptor.name()); | ||
| let oneof_name = fq_message_name.join(oneof.descriptor.name()); |
There was a problem hiding this comment.
This is good. It is better readable then the original!
| let input_type = self.resolve_ident(&input_proto_type); | ||
| let output_type = self.resolve_ident(&output_proto_type); | ||
| let input_type = | ||
| self.resolve_ident(&FullyQualifiedName::from_type_name(&input_proto_type)); |
There was a problem hiding this comment.
Does it make sense to impl From<&str> for FullyQualifiedName? Then this could do: input_proto_type.into().
| @@ -1075,13 +1073,13 @@ impl CodeGenerator<'_> { | |||
| && (fd_type == Type::Message || fd_type == Type::Group) | |||
| && self | |||
| .message_graph | |||
| .is_nested(field.type_name(), fq_message_name) | |||
| .is_nested(field.type_name(), fq_message_name.as_ref()) | |||
There was a problem hiding this comment.
Why convert to &str? is_nested could take a FullyQualifiedName as well
| @@ -19,52 +22,54 @@ fn validate_proto_path(path: &str) -> Result<(), String> { | |||
|
|
|||
| #[derive(Debug)] | |||
| pub struct ExternPaths { | |||
| // IMPROVEMENT: store as FullyQualifiedName and syn::Path | |||
There was a problem hiding this comment.
Why not use FullyQualifiedName in this PR?
| @@ -1028,7 +1026,7 @@ impl CodeGenerator<'_> { | |||
| Type::Message => Cow::Borrowed("message"), | |||
| Type::Enum => Cow::Owned(format!( | |||
| "enumeration={:?}", | |||
| self.resolve_ident(field.type_name()) | |||
| self.resolve_ident(&FullyQualifiedName::from_type_name(field.type_name())) | |||
There was a problem hiding this comment.
Should field return a FullyQualifiedName here? That would mean that the new type should be public. Maybe that is too much for this PR.
Split out from the work done in #1026