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

Do not compare types in coercion model richcmp() #24968

Closed
embray opened this issue Mar 13, 2018 · 14 comments
Closed

Do not compare types in coercion model richcmp() #24968

embray opened this issue Mar 13, 2018 · 14 comments

Comments

@embray
Copy link
Contributor

embray commented Mar 13, 2018

Though small, the fix to richcmp is significant and fixes a lot of bugs.

Component: coercion

Keywords: structure coerce

Author: Erik Bray, Jeroen Demeyer

Branch/Commit: de73aad

Reviewer: Jeroen Demeyer, Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/24968

@embray embray added this to the sage-8.2 milestone Mar 13, 2018
@jdemeyer
Copy link

comment:2

I don't like this change. It will make it harder to port Sage to Python 3 by hiding errors in an try/except block.

It would be much better to just not compare types in the first place.

@jdemeyer
Copy link

@embray
Copy link
Contributor Author

embray commented Mar 13, 2018

comment:4

If you say so--does this actually break anything?

FWIW I have personally written code before that implements comparisons between metaclasses (!). I don't understand the implications when it comes to Sage though.


New commits:

7654b19Don't compare types at all

@embray
Copy link
Contributor Author

embray commented Mar 13, 2018

Changed commit from 082ad7b to 7654b19

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 13, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

de73aadDon't compare types at all

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 13, 2018

Changed commit from 7654b19 to de73aad

@jdemeyer
Copy link

Changed author from Erik Bray to Erik Bray, Jeroen Demeyer

@jdemeyer
Copy link

comment:6

Let's see what the patchbot says.

@embray
Copy link
Contributor Author

embray commented Mar 13, 2018

comment:7

Looks like the comparison between two objects' types dates as far back to f112feb0. It's probably not relevant. Theoretically it could be useful, but probably not for any currently existing code.

@jdemeyer
Copy link

comment:8

It seems to work on the patchbot. Erik, if you agree, you can set this to positive review.

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@jdemeyer jdemeyer changed the title py3: fixes to sage.structure.coerce Do not compare types in coercion model richcmp() Mar 15, 2018
@fchapoton
Copy link
Contributor

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:10

Let us move on.

@vbraun
Copy link
Member

vbraun commented Mar 22, 2018

Changed branch from u/jdemeyer/python3/sage-structure-coerce to de73aad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants