-
Notifications
You must be signed in to change notification settings - Fork 6
deps!: Drop support for Python 3.7 and 3.8 (AI Experiment) #337
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
base: main
Are you sure you want to change the base?
Changes from all commits
9b32164
dccef48
f81c738
5cb30ca
7f44424
3e4e6f6
7beea0d
32d7b8f
e871d73
2bdf3ef
d902e31
a0543b9
6a5d943
1bbd5f7
96a5e97
8af9bee
78574c0
9ca34b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,5 +277,10 @@ def to_pandas_dtype(self): | |
|
||
|
||
# Register the type to be included in RecordBatches, sent over IPC and received in | ||
# another Python process. | ||
pa.register_extension_type(JSONArrowType()) | ||
# another Python process. Also handle potential pre-registration | ||
try: | ||
pa.register_extension_type(JSONArrowType()) | ||
except pa.ArrowKeyError: | ||
# Type 'dbjson' might already be registered if the module is reloaded, | ||
# which is okay. | ||
Comment on lines
+284
to
+285
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this could be a separate PR since it's a fix for an issue unrelated to 3.7/3.8 support. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was direct result of coverage blowing up. |
||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit too "magic" to me, but I suppose it could make testing easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was originally just in the file with no function def. We added _check_python_version...
To test against supported and unsupported versions we attempted to mock the version but timing became an issue, partly because dependencies also rely on version checks so our mock ended up preventing error-free installs of some dependencies.
We tried half a dozen ways to alter when the mock happened but none hit that sweet spot of reliably allowing the dependencies to do their thing and slipping in to allow our test to run.
Similarly, we ran into a mess of problems with import timing and mocking whether JSONArray JSONDtype did or did not import as expected and so ...
putting them in functions made things much easier in the testing arena.