Make types public #1180
Make types public #1180
Conversation
|
@LegNeato I'll look at this next week. |
There was a problem hiding this comment.
@tusharmath the proposed changes are quite controversial. Overall, I'm not a fan of them.
Could you show a code that requires these changes to achieve what you want? Having it, it would be better to come up with the solution that satisfies both of us.
| @@ -6,7 +6,7 @@ use std::{ | |||
| }; | |||
|
|
|||
| #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
| pub struct Name(String); | |||
There was a problem hiding this comment.
@tusharmath I don't like this change, because it breaks encapsulation for the Name type here and allow freely bypass its invariants. It's far better here to provide a meaningful constructor-function, which checks the invariants.
| @@ -33,8 +33,8 @@ mod ast; | |||
| pub mod executor; | |||
| mod introspection; | |||
| pub mod parser; | |||
| pub(crate) mod schema; | |||
| mod types; | |||
There was a problem hiding this comment.
@tusharmath here types are mostly re-exported to the crate's root. If something is missing, I think it's better to add that to the re-exports, rather than making these modules public.
tyranron
added
enhancement
|
I'm going to close this out. Feel free to comment if you want it reopened to continue discussion! |
tyranron
added
wontfix
Types can be used to generate a RootNode dynamically. This is a quick fix for #1117.
if this looks good, I can create a few more PRs that make some of the internal types public, allows developers to create RootNode and Schema using techniques other than Macros.