-
Notifications
You must be signed in to change notification settings - Fork 162
codegen: improve "could not find field" error message #233
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
Conversation
|
graphql_client_codegen/src/shared.rs
Outdated
.map(|ref field| &field.name) | ||
.fold(String::new(), |mut acc, item| { | ||
acc.push_str(item); | ||
acc.push_str(", "); |
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.
I think Itertools::format
would just be better.
fields.iter().map(|ref field| &field.name).format("`, `")
This keeps the field names enclosed in "`", but leaves the commas without markup.
That does look better. Added as an additional commit so it can be reviewed separately. |
graphql_client_codegen/src/shared.rs
Outdated
format_err!( | ||
"Could not find field `{}`. Available fields: `{}`.", | ||
name, | ||
format_available_fields(fields).as_str() |
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 as_str
seems superfluous now (same 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.
Right - thanks!
e81b892
to
4523ede
Compare
Looks good! We discussed the iter formatting issue on another channel - we should address that concern, rebase and then let's merge this. |
So I should rebase and then do the thing without |
yep - for the struct CommaSeparated<T>(T);
impl<A, B> std::fmt::Display for CommaSeparated<A>
where A: Iter<Item=B>,
B: Display {
....
} |
If it's too hard we can think about keeping the itertools dependency. |
@fnune the codebase has changed quite a bit since this PR was opened :) |
Is this something we should revisit? Improving error messages is almost always a good thing IMO. |
yeah definitely. We could have an issue so we don't forget |
#97
I've considered adding newlines in between but I don't know if there's any existing convention that would conflict with that.