Skip to content

MAINT: Remove sys.version checks in tests #15305

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

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

sethtroisi
Copy link
Contributor

Cleanup checks for python2 (via sys.version) in test files.

if v.shape is None:
return len(v) * v.itemsize
# Python 3.3+ uses a flexible string representation
ucs4 = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a note to simplify this and a couple of other things later

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps worth tagging all of this type of thing with a # TODO[gh-####] in the source, where gh-#### is some tracking issue that may or may not already exist.

That way, if you forget later, someone else will be able to work it out.

Comment on lines 5331 to 5332
pytest.skip('non ascii unicode field indexing skipped; '
'raises segfault on python 2.x')
Copy link
Member

Choose a reason for hiding this comment

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

Scary stuff! A little alarmed there's no issue reference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it still scary if it's going away?

I traced the origin of the code to ~9 years ago 99fc128 but it doesn't offer any explanation.

@sethtroisi
Copy link
Contributor Author

Eric's comment about EMPTY = ()
is addressed in #15307 which should be merged first.

@sethtroisi
Copy link
Contributor Author

Part of #15306

if sys.version_info.major > 2:
# map returns a list on Python 2
with assert_warns(FutureWarning):
hstack(map(lambda x: x, np.ones((3, 2))))
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you removed this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accidentally reverted it, added back, tested locally and pushed.

encoding='latin1')
assert_raises(ImportError, data.__getitem__, 'x')
data.close()
else:
Copy link
Member

Choose a reason for hiding this comment

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

too bad np.load is not a context manager

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Looks great, just @mattip's pending comment about the accidentally-removed test left to handle

@sethtroisi
Copy link
Contributor Author

@eric-wieser I fixed mattip's comment, I think this can be submitted now

@mattip mattip merged commit 36c27d8 into numpy:master Jan 16, 2020
@mattip
Copy link
Member

mattip commented Jan 16, 2020

Thanks @sethtroisi

@sethtroisi sethtroisi deleted the sys_version_tests branch January 16, 2020 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants