Skip to content
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 guess field #196

Merged
merged 10 commits into from
Feb 8, 2021
Merged

Conversation

jxnu-liguobin
Copy link
Contributor

Why are we making this change?

What effects does this change have?

I just tried, only compiled, and didn't know how to test it

Copy link
Owner

@obmarg obmarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jxnu-liguobin - thanks for making this. Looks pretty great. I've left you a few suggestions & comments - mostly rust pointers. Hope you find them helpful.

You also need to run cargo fmt on the code to get it formatted correctly - CI is complaining about that just now. You may also need to merge in master, as I fixed a broken test on master earlier today.

You mentioned testing in your commit message: as this code is mostly error messages output from macros we use trybuild to test it. There are one or two examples here currently - you can add a .rs file to the cases folder that contains a mis-spelled field or enum, then you can reference that file in a test in this file. When you run cargo test trybuild will try to compile your .rs file - you need to make sure it fails with the compile error that you added. If you then run TRYBUILD=overwrite cargo test then trybuild will commit the error output so that the tests know what error to expect.

Let me know if you want help with anything - I am happy to make some of the changes myself if you'd rather (know that getting some bits of rust right can be tricky for newcomers) or just offer advice with any problems you run into.

cynic-codegen/src/lib.rs Outdated Show resolved Hide resolved
cynic-codegen/src/enum_derive/mod.rs Outdated Show resolved Hide resolved
cynic-codegen/src/enum_derive/mod.rs Outdated Show resolved Hide resolved
cynic-codegen/src/lib.rs Outdated Show resolved Hide resolved
cynic-codegen/src/lib.rs Outdated Show resolved Hide resolved
cynic-codegen/src/lib.rs Outdated Show resolved Hide resolved
cynic-codegen/src/lib.rs Outdated Show resolved Hide resolved
cynic-codegen/src/lib.rs Outdated Show resolved Hide resolved
cynic-codegen/src/fragment_derive/mod.rs Outdated Show resolved Hide resolved
tests/ui-tests/tests/cases/enum-guess-validation.rs Outdated Show resolved Hide resolved
cynic-codegen/src/enum_derive/mod.rs Show resolved Hide resolved
tests/ui-tests/tests/cases/enum-guess-validation.stderr Outdated Show resolved Hide resolved
@obmarg obmarg added this to the v0.12.0 milestone Feb 7, 2021
Copy link
Owner

@obmarg obmarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @jxnu-liguobin - thanks again for taking it on. Got one small comment, but I may just merge and do it myself on master - wanting to get a release out today with this and a few other bits.

@@ -6,6 +6,7 @@ pub mod input_object_derive;
pub mod query_dsl;
pub mod query_module;
pub mod scalar_derive;
pub mod suggestions;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we want this module as part of the public interface of the crate, can we make it private please:

Suggested change
pub mod suggestions;
mod suggestions;

public/private in rust might work a bit different from what you're used to - private things can still be accessed by any children of the module where the private thing was declared.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@obmarg obmarg merged commit f9052b2 into obmarg:master Feb 8, 2021
obmarg added a commit that referenced this pull request Feb 8, 2021
obmarg added a commit that referenced this pull request Feb 8, 2021
@jxnu-liguobin jxnu-liguobin deleted the support-guess-field branch March 1, 2021 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants