-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
Escape stubgen names #406
Escape stubgen names #406
Conversation
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.
Some thoughts
..._stubgen/test_data/expected_outputs/use_reserved_rust_names_in_schema/adapter/schema.graphql
Show resolved
Hide resolved
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.
Nice job! There's a small-but-tricky design question that needs to be resolved first, and otherwise this is good to go.
..._stubgen/test_data/expected_outputs/use_reserved_rust_names_in_schema/adapter/schema.graphql
Show resolved
Hide resolved
485d7b4
to
4356eea
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.
Broadly looks good, just a few small things to polish up.
trustfall_stubgen/src/root.rs
Outdated
} | ||
} | ||
|
||
fn ensure_no_edge_name_keyword_conflicts( |
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.
This function can probably use a better name, more in line with the panic message below
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.
..._stubgen/test_data/expected_outputs/use_reserved_rust_names_in_schema/adapter/schema.graphql
Show resolved
Hide resolved
pub enum Vertex { | ||
Type(()), | ||
Type2(()), | ||
unsafe2(()), | ||
|
||
use_(()), | ||
|
||
use2(()), | ||
} |
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.
This will cause a lint by default because variant names should be capitalized. Let's capitalize them.
Also, probably add another test that has both upper and lower case of the same keyword at the same time, to make sure we generate a good error there.
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.
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.
especially this comment: aae96cf#diff-78e3b031bbc47a6bd07fe535e64778cc8f9030a9b983f9ebe8ef4d16ad61f581R432-R433
Resolves #392.