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

Replace abort() with raise SanicException #1520

Merged
merged 7 commits into from
Jan 5, 2022

Conversation

mildbyte
Copy link
Contributor

@mildbyte mildbyte commented Dec 28, 2021

This is for compatibility with newly-released Sanic 21.12.

Description

Sanic 21.12 removed the deprecated abort() in sanic-org/sanic#2306. Before that, it was a simple wrapper around raise SanicException (https://github.com/sanic-org/sanic/blob/523db190a732177eda5a641768667173ba2e2452/sanic/exceptions.py#L262-L265), so this change makes it explicit and removes the dependency on abort().

Note: I haven't tested this yet, relying on Strawberry's CI to catch out compat issues with the currently supported Sanic versions.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Fixes the Dependabot bump issue in #1516

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #1520 (ca59f48) into main (4b903b8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1520      +/-   ##
==========================================
+ Coverage   98.11%   98.12%   +0.01%     
==========================================
  Files         129      129              
  Lines        4403     4436      +33     
  Branches      746      754       +8     
==========================================
+ Hits         4320     4353      +33     
  Misses         43       43              
  Partials       40       40              

@botberry
Copy link
Member

botberry commented Dec 28, 2021

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Fix compatibility with Sanic 21.12

Here's the tweet text:

🆕 Release (next) is out! Thanks to Artjoms Iskovs for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

@mildbyte
Copy link
Contributor Author

I've cherry-picked ab1fe66 from the failing Dependabot PR so that we can verify this actually works when the PR is tested.

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Nice one! Thank you for this!

@patrick91
Copy link
Member

@mildbyte looks like the tests have failed, not sure if it is related to the PR or the update 🤔

would you be able to take a look at them? 😊

@mildbyte
Copy link
Contributor Author

@patrick91 Will do, looks like it's also related to some Sanic deprecations.

@mildbyte
Copy link
Contributor Author

I think this is fixed by upgrading sanic-testing to 0.8.2. I did it myself using Poetry (with poetry update --lock) but it has regenerated the lockfile with a bunch of extra changes (https://github.com/strawberry-graphql/strawberry/pull/1520/files#diff-f53a023eedfa3fbf2925ec7dc76eecdc954ea94b7e47065393dbad519613dc89). Not sure if you have a different process for updating dependencies.

mildbyte and others added 5 commits January 3, 2022 11:03
This is for compatibility with newly-released Sanic 21.12 which removed the deprecated `abort()` in sanic-org/sanic#2306. Before that, it was a simple wrapper around `raise SanicException` (https://github.com/sanic-org/sanic/blob/523db190a732177eda5a641768667173ba2e2452/sanic/exceptions.py#L262-L265), so this change makes it explicit and removes the dependency on `abort()`.
Bumps [sanic](https://github.com/sanic-org/sanic) from 21.9.3 to 21.12.0.
- [Release notes](https://github.com/sanic-org/sanic/releases)
- [Changelog](https://github.com/sanic-org/sanic/blob/main/CHANGELOG.rst)
- [Commits](sanic-org/sanic@v21.9.3...v21.12.0)

---
updated-dependencies:
- dependency-name: sanic
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
(0.7.0 uses the old `app.test_client` attribute which has been deprecated in
Sanic 21.12).
@mildbyte
Copy link
Contributor Author

mildbyte commented Jan 3, 2022

@patrick91 I need some help with getting the CI started off again, don't think I have the permission to trigger the job :)

@patrick91
Copy link
Member

@mildbyte done! will do another review in a couple of hours :)

@mildbyte
Copy link
Contributor Author

mildbyte commented Jan 3, 2022

Ugh. Looks like the test failures are now unrelated to the change directly and are due to unintended poetry.lock dependency bumps from me running poetry update --lock (I only wanted to update sanic-testing). Is there a way to just update that package?

@patrick91
Copy link
Member

@mildbyte not 100% sure! I'll take a look at it soon 😊

@patrick91
Copy link
Member

Looks like there's a small incompatibility on windows,

looks like the windows event loop implementation doesn't support passing a name:

image

I think I'll skip the test on windows 3.7 :D

@patrick91
Copy link
Member

@mildbyte are you fine with skipping the tests? if so I'll merge 😊

@mildbyte
Copy link
Contributor Author

mildbyte commented Jan 5, 2022

@patrick91 Go for it, thanks for figuring it out!

@patrick91 patrick91 merged commit 2a958ae into strawberry-graphql:main Jan 5, 2022
@botberry
Copy link
Member

botberry commented Jan 5, 2022

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

@mildbyte mildbyte deleted the patch-1 branch January 5, 2022 13:38
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