profusion / sgqlc

Simple GraphQL Client
https://sgqlc.readthedocs.io/
ISC License
506 stars 85 forks source link

codegen: sort interfaces first (fixes #189) #218

Closed benmosher closed 1 year ago

benmosher commented 1 year ago

Description

Fix #189 issue with some interfaces being sorted after their implementing object types.

I believe this is because of the way Python's list sort uses the comparator function (or maybe the comparison key conversion?) -- in my local tests it did not compare every interface to every object type and therefore some objects ended up before their implemented interfaces in the output order.

So this PR just ensures that interfaces are sorted strictly before anything else. Two interfaces or two objects fall through to the existing comparison function.

Related Issues

Fixes #189, at least my case of the same issue.

Pull request checklist

How to test it

Someone might pull and test the data hub API? maybe that can be added as a test?

Change logs

not sure what to do here?

benmosher commented 1 year ago

btw the CONTRIBUTING.md link is broken so I am not sure if my commit message conforms. 😅

barbieri commented 1 year ago

the linter is broken, the function did grow a bit long, we'll need to split it.

As for CONTRIBUTING.md and the template, they shouldn't be used by this project... someone in our company created https://github.com/profusion/.github and that started to apply to every project. It seems I need to manually provide empty templates :-/

dsnam commented 1 year ago

it would be nice to get this merged in if the fix in this branch is acceptable, are there any more changes/cleanups that need to happen first before it can move along? I am also running into this so would be happy to do them. Using the same patch as a local workaround for now.

barbieri commented 1 year ago

Hi @dsnam, sorry taking so long. I still find the approach I described at https://github.com/profusion/sgqlc/pull/209#issuecomment-1309645662 is the best solution, please see if #219 fixes your issues

dsnam commented 1 year ago

@barbieri no problem, and yep #219 also seems to fix my issue so I will keep an eye on that one. Thanks!

barbieri commented 1 year ago

@dsnam https://pypi.org/project/sgqlc/16.1/