Opened 7 years ago

Closed 6 years ago

#15983 closed enhancement (fixed)

Python 3 preparation: Change names of some function attributes

Reported by: wluebbe Owned by:
Priority: major Milestone: sage-6.2
Component: distribution Keywords: python3
Cc: chapoton Merged in:
Authors: Wilfried Luebbe, R. Andrew Ohana Reviewers: Frédéric Chapoton, R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: c03d421 (Commits) Commit: c03d421098ed3325b71384394b7920056f4d87b1
Dependencies: Stopgaps:

Description

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.

Change History (19)

comment:1 Changed 7 years ago by wluebbe

  • Branch set to u/wluebbe/ticket/15983
  • Commit set to 3b0d1f7ccde571da44651d10a94bf4c3fc448170

New commits:

3b0d1f7Change names of some function attributes

comment:2 Changed 7 years ago by wluebbe

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by chapoton

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

comment:4 Changed 7 years ago by wluebbe

  • Status changed from needs_review to needs_work

Yes, seems suspicious. I will look into it.

comment:5 Changed 7 years ago by wluebbe

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?

comment:6 Changed 7 years ago by wluebbe

  • Cc chapoton added

comment:7 Changed 7 years ago by git

  • Commit changed from 3b0d1f7ccde571da44651d10a94bf4c3fc448170 to a97c3ad9ce1e8c2f8bf11f7a61a0096eeeff2e38

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)

comment:8 Changed 7 years ago by wluebbe

  • Status changed from needs_work to needs_review

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 ...

comment:9 Changed 7 years ago by chapoton

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..

comment:10 Changed 7 years ago by wluebbe

  • Status changed from needs_review to needs_work

doctests fail in

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

comment:11 Changed 6 years ago by ohanar

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.

comment:12 Changed 6 years ago by ohanar

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.

Last edited 6 years ago by ohanar (previous) (diff)

comment:13 Changed 6 years ago by ohanar

  • Branch changed from u/wluebbe/ticket/15983 to u/ohanar/remove_func_star
  • Commit changed from a97c3ad9ce1e8c2f8bf11f7a61a0096eeeff2e38 to 9357384b4355a03e2d8c53767e601b165d6d5e6a
  • Status changed from needs_work to needs_review

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

comment:14 follow-up: Changed 6 years ago by wluebbe

  • Branch changed from u/ohanar/remove_func_star to u/wluebbe/ticket/15983
  • Commit changed from 9357384b4355a03e2d8c53767e601b165d6d5e6a to c03d421098ed3325b71384394b7920056f4d87b1
  • Status changed from needs_review to positive_review

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

comment:15 Changed 6 years ago by vbraun

author/reviewer names please

comment:16 Changed 6 years ago by wluebbe

  • Authors set to Wilfried Luebbe, R. Andrew Ohana
  • Reviewers set to Frédéric Chapoton, R. Andrew Ohana

comment:17 in reply to: ↑ 14 Changed 6 years ago by ohanar

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).

comment:18 Changed 6 years ago by wluebbe

Thanks for the explanation!

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

comment:19 Changed 6 years ago by vbraun

  • Branch changed from u/wluebbe/ticket/15983 to c03d421098ed3325b71384394b7920056f4d87b1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.