If using Python >=3.7, use built-in dict as OrderedDict

[necaris]

Since dictionaries are specified to preserve insertion order in Python 3.7 and up (see https://docs.python.org/3/whatsnew/3.7.html), it seems simplest and most performant to use the built-in type where possible.

This change does specify a distinct type, to continue to make clear that in certain situations order-preservation is relied on rather than being a convenient accident.

[necaris]

I've tried a couple of different ways to write this in a way that mypy will accept, with no luck; can anyone help me write the type signature so that mypy will agree it's compatible with OrderedDict?

[Cito]

Why don't you simply set OrderedDict = dict?

[necaris]

@Cito it looks like that's a slightly bigger change, because it changes the way that GraphQLEnumType behaves -- if not passed as an OrderedDict, currently GraphQLEnumType sorts its fields internally; with OrderedDict = dict, that will never happen, and a test fails.

I was going to look into the details of the GraphQL specification this weekend to see if that's necessary, and propose we remove that test if it's not required -- if you know already, I'd love to simplify this.

[Cito]

@necaris - you mean test_sorts_values_if_not_using_ordered_dict, right?

The question is - what is the desired behavior?

1. built-in dicts will always be auto-sorted - even in Py 3.7 where they are ordered
2. built-in dicts will be auto-sorted only for Py < 3.7

Depending on which behavior is actually desired, we need to change the test or the implementation of define_enum_value.

Personally I think solution 2 makes more sense. In that case we would need to simply skip the test for Py >= 3.7 since no "unordered" dict exists in that case.

(Btw, actually dicts are already ordered in Py 3.6, but it's only guaranteed in CPython.)

[necaris]

@Cito yes, that's exactly the test I mean.

I agree with you that solution 2 works well, and I've updated my PR to reflect that and skip the test.

However, it's still not clear what the behavior should be -- I'm sure the sorting was done for a reason, even though as far as I can see, the GraphQL spec doesn't require it at all.

[Cito]

The reason for the sorting was probably to achieve a deterministic behavior which makes testing easier. GraphQL.js uses a Map internally which is ordered.

Note that in graphql-core-next, the "values" attribute is a dict and only Py 3.6+ anyway, so the problem does not arise there.

[necaris]

@Cito sure, that could be a reason. If using a built-in dict continues to help with compatibility with the JS implementation that's even better news in my book. In any case, I hope a couple of reviewers can have a look -- do you know who I should @-mention?

[Cito]

I think @ekampf is the core maintainer. Maybe @dan98765 can also review.

[Cito]

Looks good to me.