Skip to content

Always sort scalar types by name when returning them #417

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

Closed
wants to merge 1 commit into from

Conversation

autarch
Copy link
Contributor

@autarch autarch commented Apr 2, 2022

Previously these were returned in an essentially random order. This would lead to flapping in the generated code when running the client CLI against the same schema multiple times.

In the application I'm writing, I check in the generated schema, so this sort of flapping in the generated code creates unwanted noise in the commit history.

I tested this by hand by running the fixed client against my schema a number of times. This stopped the flapping in the generated code.

@autarch
Copy link
Contributor Author

autarch commented Apr 2, 2022

I'm not sure how you feel about the itertools dep. I think the only alternative is to have the scalars method call collect into a Vec, then sort the vec and return sorted_vec.into_iter(). I can implement it that way if you prefer. This is exactly what itertools does under the hood.

@tomhoule
Copy link
Member

tomhoule commented Apr 4, 2022

Itertools avoids allocating a whole vec, but it's very easy to reimplement (see https://github.com/prisma/prisma-engines/blob/fe68ee16ec7694924b82bf075382fa68e69989da/migration-engine/connectors/sql-migration-connector/src/sql_renderer/common.rs#L92 for example) — I'd rather avoid taking on a new dependency so let's add this trait to graphql-client.

The PR looks good!

@autarch autarch force-pushed the autarch/sort-scalars branch from 62054bc to 37ec1ab Compare April 5, 2022 02:25
@autarch
Copy link
Contributor Author

autarch commented Apr 5, 2022

I updated the PR to do the sorting "by hand". It still has to allocate the Vec. I'm not sure how the example you linked would help avoid that, since it's generating a string rather than returning a sorted Vec or iterator.

@autarch
Copy link
Contributor Author

autarch commented Apr 5, 2022

Actually, looking more closely at the code I think it'd be possible to have this problem with inputs and enums too, so I'll do the same thing for them.

Previously these were returned in an essentially random order. This would lead
to flapping in the generated code when running the client CLI against the same
schema multiple times.

In the application I'm writing, I check in the generated schema, so this sort
of flapping in the generated code creates unwanted noise in the commit
history.
@autarch autarch force-pushed the autarch/sort-scalars branch from 37ec1ab to b708e80 Compare April 5, 2022 02:32
@autarch
Copy link
Contributor Author

autarch commented Apr 5, 2022

The CI failures look to be unrelated to this PR. There is some code in main that isn't properly rustfmt'd. If it got to clippy that would also fail because of some unused struct fields. The prettier failure is also because of files in main that aren't properly formatted.

@autarch
Copy link
Contributor Author

autarch commented Apr 15, 2022

ping

@LegNeato
Copy link
Member

Is this obsoleted by #430 ?

@tomhoule
Copy link
Member

I think it is

@autarch
Copy link
Contributor Author

autarch commented Sep 3, 2022

I don't understand the code base well enough to say if #430 obsoletes this one, but I'm fine with closing this if it does.

.inputs()
.filter(move |(id, _input)| self.types.contains(&TypeId::Input(*id)))
.collect::<Vec<_>>();
inputs.sort_by_key(|(_id, input)| input.name.as_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using Itertools::sorted_by_key can avoid the intermediate Vec.

@mathstuf
Copy link
Contributor

Ah, I see. #430 probably does resolve this. Tested on main by fetching the Github schema three times and getting the same result each time.

@mathstuf mathstuf closed this Oct 13, 2023
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.

4 participants