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

Bump ns1-python version to 0.18.0 #23

Merged
merged 1 commit into from
Oct 13, 2022
Merged

Conversation

istr
Copy link
Contributor

@istr istr commented Oct 11, 2022

Upstream had accepted my PR to support python 3.10 in https://github.com/ns1/ns1-python

@ross
Copy link
Contributor

ross commented Oct 11, 2022

Looks like PR that was merged is ns1/ns1-python#107.

Little confused though as it seems like we already have 3.10 in our CI list,

python-version: ['3.7', '3.8', '3.9', '3.10']

was the fixed problem only an issue at actual runtime and not when things were stubbed for CI?

Looking at the change to ns1's record file, https://github.com/ns1/ns1-python/pull/107/files#diff-46d1e6289881ceb01279f697ff44b91f4f0b22abf467f8bfa709397cc51f4a19, it seems like that'd blow up regardless.

@istr
Copy link
Contributor Author

istr commented Oct 13, 2022

Looks like PR that was merged is ns1/ns1-python#107.

Little confused though as it seems like we already have 3.10 in our CI list,

python-version: ['3.7', '3.8', '3.9', '3.10']

was the fixed problem only an issue at actual runtime and not when things were stubbed for CI?

Looking at the change to ns1's record file, https://github.com/ns1/ns1-python/pull/107/files#diff-46d1e6289881ceb01279f697ff44b91f4f0b22abf467f8bfa709397cc51f4a19, it seems like that'd blow up regardless.

Indeed, the problem occurs only at actual runtime. The testsuite mocks the relevant code like this:

    @patch('ns1.rest.records.Records.update')
    @patch('ns1.rest.records.Records.create')

Only these two call the offending _getAnswersForBody internal function.

And yes, the fix in ns1-python 0.18.0 really fixes the issue. 😃

What blew up in the unpatched version was the call to

        elif not isinstance(answers, collections.Iterable):

that tried to "resolve" .Iterable only at runtime (so the module could be loaded, but the code blew up when executed).

Resolving at load time using

try:
    from collections.abc import Iterable

for 3.10 and later and falling back to

except:
    from collections import Iterable

for 2.7 resolves the issue at load time.

@istr
Copy link
Contributor Author

istr commented Oct 13, 2022

And just a general thought on mocking: If you mock something in tests, you should always do thorough isolated/separate tests against the original, "non-mocked" material so you can find out if it misbehaves. 😃

@ross ross merged commit 6d57dbf into octodns:main Oct 13, 2022
@ross
Copy link
Contributor

ross commented Oct 14, 2022

Started to cut a new release for this, but realized there's no reason to since users are generally in charge of selecting/pinning the versions of libs they use. If we'd bumped the minimum requirement in setup.py to be >= 0.18.0 it'd make sense, but otherwise no need.

@istr istr deleted the ns1-python-310-support branch October 20, 2022 17:15
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.

2 participants