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

Release 0.11.0 #108

Merged
merged 43 commits into from
Jul 11, 2023
Merged

Release 0.11.0 #108

merged 43 commits into from
Jul 11, 2023

Conversation

tOgg1
Copy link
Owner

@tOgg1 tOgg1 commented Jun 17, 2023

Preparation of the 0.11.0 release.

@mbuvarp Would you be so kind as to do a final review?

rymanso and others added 23 commits February 3, 2022 11:16
Also updates black version used, and sets max line length of 120
Bumps [django](https://github.com/django/django) from 2.2.27 to 2.2.28.
- [Release notes](https://github.com/django/django/releases)
- [Commits](django/django@2.2.27...2.2.28)

---
updated-dependencies:
- dependency-name: django
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
`many_to_one_extras` -> `many_to_many_extras` in the relevant section.
Bumps [sqlparse](https://github.com/andialbrecht/sqlparse) from 0.4.2 to 0.4.4.
- [Release notes](https://github.com/andialbrecht/sqlparse/releases)
- [Changelog](https://github.com/andialbrecht/sqlparse/blob/master/CHANGELOG)
- [Commits](andialbrecht/sqlparse@0.4.2...0.4.4)

---
updated-dependencies:
- dependency-name: sqlparse
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Allow overwriting return ids in Batch Delete
@tOgg1 tOgg1 requested a review from mbuvarp June 17, 2023 10:53
Copy link
Contributor

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

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

These changes look good to me! I pip-installed this branch into a medium/large-sized project where I have dozens of unit-tests for various mutation behavior that utilizes graphene-django-cud (Create, BatchCreate, Patch, and Delete specifically) and all tests still pass.

I think it may be worth trying to merge #110 (and potentially #111 as well, since that seems to resolve an issue several people ran into), if possible. Thanks again for reviving graphene-django-cud and pushing another release forward 🙏

Copy link
Collaborator

@mbuvarp mbuvarp left a comment

Choose a reason for hiding this comment

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

Looks good to me, aside from all the checks failing (likely just because ubuntu-18.04 is deprecated, is probably fine just changing to ubuntu-latest).

tOgg1 added 9 commits July 1, 2023 20:55
Change ubuntu version from 18 to 22
Remove testing for python version 3.6, and add for python version 3.9
and 3.10.
- Change ubuntu running versions from 22 to 20 (for potential compatability issues with older versions of python in Ubuntu 22)
- Make python versions in matrix be strings
- Make release-task use ubuntu 20 and python version 3.9
- Upgrade to newer way of installation poetry
- Change caching strategy to doing it directly in the python setup
- Update some packages further
- Fix issues after updating factoryboy
Fix issue #81 and #105 properly, by amending the way we handle already-converted choices field from graphene-django 3.0.

The fix introduced in #111 made the error people experienced from the bad handling go away, but it didn't actually set the `is_required` flag correctly. This is because mounted BlankValue Field actually wrapped the underlying choices enum in `NonNull` when pertinent.

Ths issue was related to graphene-django 3.0 taking the converted enum class and mounting it outright in a BlankValueField, instead of just saving the enum itself.

We solve this by, for graphene-django 3.0, by retrieving the wrapped choices Enum, and reusing this.
@tOgg1
Copy link
Owner Author

tOgg1 commented Jul 2, 2023

Tests are running successfully again. Getting close to the release.

The pre-commits are still failing. Care to check that out @mbuvarp?

@sjdemartini Is your medium/large-size repo using graphene-django 2.x? If so, could you run another quick check from the latest HEAD here? I've had to implement another quick fix for the issue outlined in #81 and #105.

@sjdemartini
Copy link
Contributor

@tOgg1 Thanks for all the effort here. We upgraded to graphene-django v3 a few weeks ago (though not the latest patch version yet due to the issue in #109). I haven't seen the error that was mentioned in #81 and #105. I noticed both of the folks who reported it were using Django 4, and we're using 3.2, so perhaps that accounts for the difference, but I haven't looked into it in any detail. I can try to investigate more what would be causing that if you'd like.

@sjdemartini
Copy link
Contributor

Oh, it occurred to me just now that what's likely the key difference is that I use a separate enum class which I manually define for each of my graphene_django object types (since I want more control over enum naming whenever I use choices in Django, rather than the default auto-generated choice enums). I then specify those enum classes via field_types overrides in my CUD mutations, so I imagine my app never executes the convert_django_field_with_choices function that needed fixing.

@tOgg1
Copy link
Owner Author

tOgg1 commented Jul 3, 2023

Thanks, @sjdemartini!

I think it's very likely just a graphene-django v3 vs graphene-django v2 issue. I'll run some tests myself with graphene-django 2.0, and see if anything breaks.

Fix compatibility issue with importing BlankValueField and assert_name from graphql

Signed-off-by: Tormod Haugland <tormod.haugland@gmail.com>
@tOgg1
Copy link
Owner Author

tOgg1 commented Jul 3, 2023

It broke indeed. But I think I've got it working now.

Remaining checklist before release:

  • Fix pre-commit workflow @mbuvarp
  • Test the latest commit in a large'ish repository. Preferably on both graphene-django 2.x and 3.x. @tOgg1

@sjdemartini
Copy link
Contributor

Hm, I get a new error trying the latest commit:

ImportError while importing test module '/path/to/myapp/tests/test_mutations.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../.pyenv/versions/3.10.5/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
myapp/tests/test_mutations.py:6: in <module>
    from myapp.mutations import MyModel
myapp/mutations.py:2: in <module>
    from graphene_django_cud.mutations import DjangoCreateMutation, DjangoPatchMutation
venv/lib/python3.10/site-packages/graphene_django_cud/mutations/__init__.py:1: in <module>
    from .batch_create import DjangoBatchCreateMutation
venv/lib/python3.10/site-packages/graphene_django_cud/mutations/batch_create.py:13: in <module>
    from graphene_django_cud.mutations.core import DjangoCudBase, DjangoCudBaseOptions
venv/lib/python3.10/site-packages/graphene_django_cud/mutations/core.py:10: in <module>
    from graphene_django_cud.util import (
venv/lib/python3.10/site-packages/graphene_django_cud/util/__init__.py:1: in <module>
    from .model import *  # noqa: F401 F403
venv/lib/python3.10/site-packages/graphene_django_cud/util/model.py:16: in <module>
    from graphene_django_cud.converter import (
venv/lib/python3.10/site-packages/graphene_django_cud/converter.py:40: in <module>
    from graphql import GraphQLError, assert_name
E   ImportError: cannot import name 'assert_name' from 'graphql' (/path/to/venv/lib/python3.10/site-packages/graphql/__init__.py)

These are the relevant package versions I'm using:

graphql-core==3.1.7
graphene==3.2.2
graphene-django==3.0.2
graphene-django-cud @ git+https://git@github.com/tOgg1/graphene-django-cud.git@3a5e2324b68f6df436a06699b108872ed372b919

It seems graphql-core changed assert_valid_name to assert_name in v3.2 (graphql-python/graphql-core@9f35a7a). So that particular condition here:

if graphql.__version__.startswith("3"):
from graphql import GraphQLError, assert_name

should seemingly be changed to be specific to 3.2+ (not 3+).

Side note: I was already under the impression that graphene-django-cud dropped support for graphene-django 2 in its 0.10.0 release due to its version specification

graphene-django = "^3.0.0b7"
so perhaps it's fine to skip testing graphene_django v2 (folks using that could stick with <0.10.0 presumably). That seems to be the trend with most graphene-django third party packages, where they dropped support for v2 when adding support for v3.

Plebbimon and others added 4 commits July 11, 2023 16:30
Change the target graphql version from 3 to 3.2 when differing on whether or not to use "assert_name" or "assert_valid_name"

Signed-off-by: Tormod Haugland <tormod.haugland@gmail.com>
Signed-off-by: Tormod Haugland <tormod.haugland@gmail.com>
@tOgg1 tOgg1 merged commit 6cb0045 into master Jul 11, 2023
@tOgg1
Copy link
Owner Author

tOgg1 commented Jul 11, 2023

Released. Thanks for all the help guys!

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.

8 participants