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

Python 3 preparation: Change names of some function attributes #15983

Closed
wluebbe mannequin opened this issue Mar 20, 2014 · 25 comments
Closed

Python 3 preparation: Change names of some function attributes #15983

wluebbe mannequin opened this issue Mar 20, 2014 · 25 comments

Comments

@wluebbe
Copy link
Mannequin

wluebbe mannequin commented Mar 20, 2014

Only the modern syntax like f.__doc__ is accepted by Python 3.

Changes according to lib2to3/fixes/fix_funcattrs.py:

f.func_x -> f.__x__
for 
('func_closure' | 'func_doc' | 'func_globals' | 'func_name' | 
'func_defaults' | 'func_code' | 'func_dict')

This ticket is tracked as a dependency of meta-ticket ticket:15980.

CC: @fchapoton

Component: distribution

Author: Wilfried Luebbe, R. Andrew Ohana

Branch/Commit: c03d421

Reviewer: Frédéric Chapoton, R. Andrew Ohana

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

@wluebbe wluebbe mannequin added this to the sage-6.2 milestone Mar 20, 2014
@wluebbe wluebbe mannequin added c: distribution labels Mar 20, 2014
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 20, 2014

Commit: 3b0d1f7

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 20, 2014

New commits:

3b0d1f7Change names of some function attributes

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 20, 2014

Branch: u/wluebbe/ticket/15983

@wluebbe wluebbe mannequin added the s: needs review label Mar 20, 2014
@fchapoton
Copy link
Contributor

comment:3

maybe you also have to correct the names inside the 'hasattr' ?

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 20, 2014

comment:4

Yes, seems suspicious. I will look into it.

@wluebbe wluebbe mannequin added s: needs work and removed s: needs review labels Mar 20, 2014
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 21, 2014

comment:5

From the Python docs (http://docs.python.org/2/reference/datamodel.html):

//Changed in version 2.6: The double-underscore attributes !closure!, !code!, !defaults!, and !globals! were introduced as aliases for the corresponding func_* attributes for forwards compatibility with Python 3.//

!dict!, !doc! and !name! were already available earlier.

Looking at the code with hasattr it should probably be manually refactored, keeping in mind that the __XX__ are always available as alias for the func_XX.

Would you agree?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2014

Changed commit from 3b0d1f7 to a97c3ad

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 22, 2014

Branch pushed to git repo; I updated commit sha1. New commits:

a97c3adadded manual changes that were not found by 2to3 tool (in .pyx and hasattr)

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 22, 2014

comment:8

The modules with hasattr() might still benefit from some refactoring, as the code could now be a bit simplified.

But I leave this for later ...

@wluebbe wluebbe mannequin added s: needs review and removed s: needs work labels Mar 22, 2014
@fchapoton
Copy link
Contributor

comment:9

looks good, I have checked that every instance was found and corrected. But one needs to make sure that nothing is broken and that all tests pass..

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 27, 2014

comment:10

doctests fail in

  • src/sage/sets/set_from_iterator.py (line 438)
  • src/sage/misc/cachefunc.pyx (line 315 and 318)

@wluebbe wluebbe mannequin added s: needs work and removed s: needs review labels Mar 27, 2014
@ohanar
Copy link
Member

ohanar commented Apr 3, 2014

comment:11

Actually this looks like a bug fix -- the previous version wouldn't work on Python functions/methods defined in Cython code (well they really just duck-type Python functions), since Cython doesn't include the func_* aliases.

The doctests just need updating to reflect the bug fix.

@ohanar
Copy link
Member

ohanar commented Apr 4, 2014

comment:12

Actually, nevermind. This is supposed to be stripped, but there are now two instances of this line information, and only one is getting stripped. I should have a patch soon to fix it.

It seems that the 'func_doc' was being used to separate cython functions from python functions.

@ohanar
Copy link
Member

ohanar commented Apr 4, 2014

comment:13

Ok, I believe this should be a fix for the issue, for now.


New commits:

9357384remove hack that used func_doc to distinguish cython and python functions

@ohanar
Copy link
Member

ohanar commented Apr 4, 2014

Changed commit from a97c3ad to 9357384

@ohanar
Copy link
Member

ohanar commented Apr 4, 2014

Changed branch from u/wluebbe/ticket/15983 to u/ohanar/remove_func_star

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

Changed commit from 9357384 to c03d421

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

Changed branch from u/ohanar/remove_func_star to u/wluebbe/ticket/15983

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

comment:14

I did a rebase on develop to test with a fresh 6.2.beta6.

MAKE='make -j4' make
./sage -tp 4 --all --long >logs/sage-ptestlong-develop
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 2379.2 seconds
#####
git fetch trac u/ohanar/remove_func_star:u/ohanar/remove_func_star
git co u/ohanar/remove_func_star
git co -b u/wluebbe/ticket/15983
git rebase develop
#####
./sage -b
./sage -tp 4 --all --long >logs/sage-ptestlong-15983
All tests passed!

My questions (as still being pretty new to Sage):

  • Is it OK to build and test as above?
  • When is it necessary to do make clean; make lib-clean; make instead of ./sage -b?

New commits:

2908c63Change names of some function attributes
108ecccadded manual changes that were not found by 2to3 tool (in .pyx and hasattr)
c03d421remove hack that used func_doc to distinguish cython and python functions

@vbraun
Copy link
Member

vbraun commented Apr 4, 2014

comment:15

author/reviewer names please

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

Reviewer: Frédéric Chapoton, R. Andrew Ohana

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

Author: Wilfried Luebbe, R. Andrew Ohana

@ohanar
Copy link
Member

ohanar commented Apr 4, 2014

comment:17

Replying to @wluebbe:

I did a rebase on develop to test with a fresh 6.2.beta6.

In general, you should probably merge in the latest beta rather than rebase (this is a change from how sage use to do things to match standard git practices). That said, extra merges are unnecessary, so you should try to avoid making merges unless there is a merge conflict or an issue is only popping up on a more recent version of sage.

My questions (as still being pretty new to Sage):

  • Is it OK to build and test as above?

Yes, although part of a review is to look at the changes in the code and make sure you agree with them.

  • When is it necessary to do make clean; make lib-clean; make instead of ./sage -b?

Use make if there have been upgraded packages in the distribution (so between most betas/releases), you can use ./sage -b if the only changes between the last time you built have been to the sage library (useful if you are working on a feature branch that only touches the sage library).

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

comment:18

Thanks for the explanation!

Next time I will try a merge when there is a merge conflict.

@vbraun
Copy link
Member

vbraun commented Apr 4, 2014

Changed branch from u/wluebbe/ticket/15983 to c03d421

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

3 participants