8000 MAINT: Remove sys.version checks by sethtroisi · Pull Request #15335 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Remove sys.version checks #15335

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 21, 2020
Merged

MAINT: Remove sys.version checks #15335

merged 1 commit into from
Jan 21, 2020

Conversation

sethtroisi
Copy link
Contributor
@sethtroisi sethtroisi commented Jan 16, 2020 •
edited
Loading

More sys.version cleanups (these are not limited to test files like #15305 was), after this change only a handful sys.versions exist which I'll add remove/convert/simplify in a final PR.

#15306

eric-wieser
eric-wieser reviewed Jan 16, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Jan 16, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Jan 16, 2020
View reviewed changes
mattip
mattip reviewed Jan 16, 2020
View reviewed changes
Copy link
Member
@mattip mattip left a comment

Choose a reason for hiding this comment

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

Only a few style nits. In general, I tried to flag import builtins, it usually indicates a hacky solution to a problem and causes a very small performance penalty. Also, there are some strange import semantics where python2 is using relative imports but python3 changed to absolute.

sethtroisi
sethtroisi commented Jan 16, 2020
View reviewed changes
@seberg seberg mentioned this pull request Jan 17, 2020
Closed
mwtoews
mwtoews suggested changes Jan 17, 2020
View reviewed changes
Copy link
Contributor
@mwtoews mwtoews left a comment

Choose a reason for hiding this comment

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

Just a few imports to remove or sort/group at the top

@sethtroisi
Copy link
Contributor Author

These are wonderful comments mwtoews I'm on vacation but will fix when I'm back

@mattip
Copy link
Member
mattip commented Jan 20, 2020

There are merge conflicts

mattip
mattip reviewed Jan 20, 2020
View reviewed changes
Copy link
Member
@mattip mattip left a comment

Choose a reason for hiding this comment

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

There are some comments still open, and a few follow-on comments as well.

@sethtroisi
Copy link
Contributor Author

Addressed @mattip's comments, fixed merge conflict, and did another quick pass over all the changes.

@mattip mattip merged commit f71d993 into numpy:master Jan 21, 2020
@mattip
Copy link
Member
mattip commented Jan 21, 2020

Thanks @sethtroisi

@sethtroisi sethtroisi deleted the sys_version_easy branch January 21, 2020 00:36
@sethtroisi
Copy link
Contributor Author

Thanks for all the comments and pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg left review comments

@eric-wieser eric-wieser eric-wieser left review comments

@mattip mattip mattip left review comments

@mwtoews mwtoews mwtoews approved these changes

Assignees
No one assigned
Labels
03 - Maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sethtroisi @mattip @seberg @eric-wieser @mwtoews
0