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

Add Python 3.12 compatibility for path fieldtype #91

Merged
merged 4 commits into from
Jan 4, 2024

Conversation

yunzheng
Copy link
Member

Fixes #89

@yunzheng yunzheng requested a review from pyrco October 20, 2023 08:45
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a9808ec) 80.08% compared to head (688803a) 80.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   80.08%   80.12%   +0.03%     
==========================================
  Files          33       33              
  Lines        3113     3119       +6     
==========================================
+ Hits         2493     2499       +6     
  Misses        620      620              
Flag Coverage Δ
unittests 80.12% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flow/record/fieldtypes/__init__.py Show resolved Hide resolved
if PY_312:
# Starting from Python 3.12, pathlib._Flavours are removed as you can now properly subclass pathlib.Path
# See also: https://github.com/python/cpython/issues/88302
PosixFlavour = pathlib.PurePosixPath
Copy link
Contributor

Choose a reason for hiding this comment

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

The _flavour property in 3.12 is a link to the posixpath or ntpath module. We could do something similar by having a function def custom_flavour(sep, altsep) that yields a patch()ed posixpath with the given separator and alt separator and modify PureCustomPath like:

    class PureCustomPath(pathlib.PurePath):
        if PY_312:
            _flavour = custom_flavour(sep, altsep)
        else:
            _flavour = CustomFlavour()

Our tests however just need a PurePath subclass instance with a _flavour property that has sep and altsep properties, so a simpler setup would for this case also work.

Copy link
Member Author

@yunzheng yunzheng Oct 23, 2023

Choose a reason for hiding this comment

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

i'm happy to accept any code suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say, just do

Suggested change
PosixFlavour = pathlib.PurePosixPath
PosixFlavour = Mock

as we probably don't want to really mock posixpath and now it is still clear that flavour is something different.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change gives me an error:

.tox/py3/lib/python3.12/site-packages/_pytest/assertion/rewrite.py:178: in exec_module
    exec(co, module.__dict__)
tests/test_fieldtypes.py:558: in <module>
    (custom_pure_path(sep="/", altsep="")("/foo/bar"), True),
tests/test_fieldtypes.py:547: in custom_pure_path
    class PureCustomPath(pathlib.PurePath):
tests/test_fieldtypes.py:548: in PureCustomPath
    _flavour = CustomFlavour()
tests/test_fieldtypes.py:543: in __new__
    instance.sep = sep
/usr/local/lib/python3.12/unittest/mock.py:771: in __setattr__
    elif (self._spec_set and self._mock_methods is not None and
/usr/local/lib/python3.12/unittest/mock.py:656: in __getattr__
    elif self._mock_methods is not None:
/usr/local/lib/python3.12/unittest/mock.py:655: in __getattr__
    raise AttributeError(name)
E   AttributeError: _mock_methods

Copy link
Member Author

Choose a reason for hiding this comment

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

@pyrco any ideas?

@yunzheng yunzheng requested a review from pyrco October 26, 2023 20:48
@yunzheng yunzheng force-pushed the improvement/pathlib-flavour-compat branch from ab7f7e8 to da57231 Compare October 26, 2023 21:03
@pyrco
Copy link
Contributor

pyrco commented Oct 31, 2023

I had this change in mind. It's not a real mock of posixpath/ntpath but for the testcase purposes it is sufficient.

@pyrco pyrco force-pushed the improvement/pathlib-flavour-compat branch 2 times, most recently from 6fb4f36 to 7e2966f Compare October 31, 2023 10:31
@pyrco
Copy link
Contributor

pyrco commented Oct 31, 2023

Ok, this didn't work as expected, I'll have to look a bit more into it

@pyrco pyrco force-pushed the improvement/pathlib-flavour-compat branch 3 times, most recently from 453930b to 34aa18f Compare October 31, 2023 10:48
@yunzheng
Copy link
Member Author

Ok, this didn't work as expected, I'll have to look a bit more into it

any luck with this?

@Schamper
Copy link
Member

Schamper commented Jan 3, 2024

I've pushed a possible fix for the unit test.

@pyrco
Copy link
Contributor

pyrco commented Jan 4, 2024

Yes, that is the mock I was looking for 😉

@yunzheng yunzheng force-pushed the improvement/pathlib-flavour-compat branch from f99737c to 688803a Compare January 4, 2024 09:30
@yunzheng yunzheng merged commit 58d5915 into main Jan 4, 2024
32 checks passed
@yunzheng yunzheng deleted the improvement/pathlib-flavour-compat branch January 4, 2024 09:57
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.

_PosixFlavour is removed in Python 3.12
3 participants