Skip to content

Revert the removal of the _value postfix in the graphql() function #234

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
Cito opened this issue Mar 19, 2019 · 2 comments
Closed

Revert the removal of the _value postfix in the graphql() function #234

Cito opened this issue Mar 19, 2019 · 2 comments

Comments

@Cito
Copy link
Member

Cito commented Mar 19, 2019

In f6d79ab, the _value postfix of some of the graphql() parameters has been removed and gives a deprecation warning now.

I propose to revert that change (and reverse the deprecation message, i.e. display it now for the shorter names instead), because:

  • We want to be as compatible with GraphQL.js (the "GraphQL reference implementation") as possible, including the naming. This makes it easier for us to keep in sync with GraphQL.js in the future and for developers to translate examples directly from JavaScript to Python (you only need to translate from camelCase to snake_case, but not change names).
  • The _value postfix still exists in the ExecutionContext which is inconsistent
  • graphql-core-next and graphql-core/modern branch also use the names with the _value postfix

If we believe the _value postfix is really stupid and unnecessary or want to make other name changes, then please in the future let's first try to convice the GraphQL.js people (who are nice and responsive) to make that change there, and then we can replicate it in Python.

@syrusakbary - what do you think?

@dan98765
Copy link
Collaborator

My understanding is that graphql-core's purpose is to match the js impl as closely as possible. Even in cases where that makes it a bit un-pythonic it makes it easier to keep it up-to-date with the reference implementation in js. I think that's one of the reasons graphene even exists: as a more pythonic, easier-to-use layer on top of graphql-core.

So I'm fine with making the variable names a bit worse (in my opinion) in order to make it easier to stay up-to-date. Also good to have this be close-ish to graphql-core-next so it's easier for people to switch over to that. I don't think _value is even important enough to talk to graphql.js people about.

Cito added a commit that referenced this issue Dec 31, 2019
This restores the `_value(s)` prefix of three parameters of the
execute() and graphql() functions, which had been removed in v2.1,
in order to be compatible with graphql-core-next (v3), graphql-js,
and the older versions again.

The prefix-less parameters are still accepted, but now produce
deprecation warnings (in v2.1 it was the other way around).
@Cito
Copy link
Member Author

Cito commented Dec 31, 2019

Ok, so I have reverted this change now. Starting with v2.3 the old parameter names will be valid again. The short parameter names still work, but will now produce deprecation warnings.

@Cito Cito closed this as completed Dec 31, 2019
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

No branches or pull requests

2 participants