Skip to content
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

WIP: Graphql next #905

Closed
wants to merge 5 commits into from
Closed

WIP: Graphql next #905

wants to merge 5 commits into from

Conversation

ulgens
Copy link
Collaborator

@ulgens ulgens commented Mar 15, 2020

This PR is based on #904 . When it's merged, this branch should be rebased from main branch.

I tried to use existing PRs #812 and #774 and complete what is left. There is no proper organization in commits at the moment, i'll edit the branch in couple of days.

There are several things to discuss but it passes all tests and i think this is a good start.

@ulgens ulgens mentioned this pull request Mar 15, 2020
7 tasks
@jkimbo
Copy link
Member

jkimbo commented Mar 18, 2020

This is great thank you @ulgens ! At the moment we are still working on getting Graphene v3 out so this might have to hang around for a bit until that happens.

@rhizoome rhizoome mentioned this pull request Apr 1, 2020
1 task
@rhizoome rhizoome mentioned this pull request Apr 6, 2020
@ulgens
Copy link
Collaborator Author

ulgens commented Apr 6, 2020

@ganwell On Travis, all of the test configs were called and they all pass except the ones allowed to fail: https://travis-ci.org/github/graphql-python/graphene-django/builds/663374515

@rhizoome
Copy link
Contributor

rhizoome commented Apr 6, 2020

@ganwell On Travis, all of the test configs were called and they all pass except the ones allowed to fail: https://travis-ci.org/github/graphql-python/graphene-django/builds/663374515

That is a test with broken tox.ini:

py{36,37,38}-django{111,django22,django30,master},

I appreciate all the work you have done. And the reason why I want to start fresh from master is because I don't understand what is going on. I am sorry that I haven't much time the last two month. I just want to use the time I have to dig trough #905 and #774 by applying these slowly to master.

@ulgens
Copy link
Collaborator Author

ulgens commented Apr 6, 2020

@ganwell I created #904 with that mindset, and @jkimbo recommended that we should create a "3" branch and merge all of it in that. I'm okay with that path, and i'm not really sure applying these changes to master incrementally is an option. Without all the changes in this branch, you will always have at least one failing test.

@rhizoome
Copy link
Contributor

rhizoome commented Apr 6, 2020

If you look at this:

https://travis-ci.org/github/graphql-python/graphene-django/jobs/663374517

Nothing is tested. tox doesn't do anything.

@rhizoome
Copy link
Contributor

rhizoome commented Apr 6, 2020

I'm okay with that path, and i'm not really sure applying these changes to master incrementally is an option.

I will mark the tests that fail as pytest.mark.xfail and then stepwise work on these, till no pytest.mark.xfail are left

@ulgens
Copy link
Collaborator Author

ulgens commented Apr 6, 2020

@ganwell Okay, thank you. That was the kind of explanation i'm looking for about what is wrong. Checking it now.

@ulgens
Copy link
Collaborator Author

ulgens commented Apr 6, 2020

I will mark the tests that fail as pytest.mark.xfail and then stepwise work on these, till no pytest.mark.xfail are left

That means we're going to ship a broken package with tests allowed to fail. I fixed tox.ini and saw the errors you mentioned before, all of them can be fixed in a day.

@rhizoome
Copy link
Contributor

rhizoome commented Apr 6, 2020

That means we're going to ship a broken package with tests allowed to fail. I fixed tox.ini and saw the errors you mentioned before, all of them can be fixed in a day.

That would be great!! Thank you very much. I only proposed the slow way, because I don't understand graphene-django well enough. So I will work on the flaky tests on master instead, while you fix #905. I am glad we can avoid the slow way.

@ulgens
Copy link
Collaborator Author

ulgens commented Apr 6, 2020

@ganwell Thank you 🙂

So, for the anyone who is looking for the result of that conv., i'm pulling that PR to WIP status, will fix rest of the broken tests and hopefully, we will good to go tomorrow.

@ulgens ulgens changed the title Graphql next WIP: Graphql next Apr 6, 2020
@rhizoome
Copy link
Contributor

rhizoome commented Apr 6, 2020

That means we're going to ship a broken package with tests allowed to fail. I fixed tox.ini and saw the errors you mentioned before, all of them can be fixed in a day.

We would make PRs to a branch called graphe-django-3.0, which will follow master and we would only merge to master once everything is fixed. This way we have small code-reviews and we don't pollute master. But hopefully this isn't needed.

@ulgens ulgens changed the base branch from master to v3 April 26, 2020 13:53
ulgens added 5 commits April 26, 2020 16:56
GraphQLError("Name '__debug' must not begin with '__', which is reserved
by GraphQL introspection."
…ray_slice

connection_from_list_slice:
> Deprecated alias for connection_from_array_slice. We're now using the
JavaScript terminology in Python as well, since list is too narrow a
type and there is no other really appropriate type name.

https://github.com/graphql-python/graphql-relay-py/blob/v3.0.0/src/graphql_relay/connection/arrayconnection.py#L54
@jkimbo
Copy link
Member

jkimbo commented May 9, 2020

Closing this because #951 has been merged into the v3 branch.

@jkimbo jkimbo closed this May 9, 2020
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.

3 participants