py3: minor fixes to sage.misc.cachefunc

[embray]

With these fixes, plus various fixes in other modules (to be submitted separately) I was able to get all the tests in sage.misc.cachefunc to pass.

Component: python3

Author: Erik Bray

Branch/Commit: 1a60988

Reviewer: Frédéric Chapoton, Jeroen Demeyer

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

[embray]

comment:1

As an aside, I noticed that the argument classmethod to CachedFunction is an unfortunate misnomer. I wonder if we couldn't do something about that (separately); maybe rename it just method or ismethod and deprecate the old name...

[jdemeyer]

comment:2

Replying to @embray:

As an aside, I noticed that the argument classmethod to CachedFunction is an unfortunate misnomer.

Is that a problem? It's not that uncommon to have keyword arguments which are also builtin functions.

[jdemeyer]

comment:3

As another aside, functions like keys() would better use yield instead of returning lists. I'm not saying that you must fix this though...

[jdemeyer]

comment:4

Is there any particular reason that you use

for k in list(self):

instead of

for k in self:

[embray]

comment:5

Replying to @jdemeyer:

Replying to @embray:

As an aside, I noticed that the argument classmethod to CachedFunction is an unfortunate misnomer.

Is that a problem? It's not that uncommon to have keyword arguments which are also builtin functions.

The problem is that the argument refers simply to normal methods, not to classmethod as in the Python built-in.

[embray]

comment:6

Replying to @jdemeyer:

As another aside, functions like keys() would better use yield instead of returning lists. I'm not saying that you must fix this though...

I agree--I could fix that while I'm updating this code anyways.

[jdemeyer]

comment:7

So what is the plan here? I'd like you to fix [comment:4] as a minimum (or explain why the list() is a good idea).

[embray]

comment:8

The list() is just force of habit from working with actual dicts. Obviously in this case it's not necessary. If the keys() method is turned into a generator then it makes sense.

[fchapoton]

Changed commit from 49a4e2f to 1a60988

[fchapoton]

Changed branch from u/embray/python3/sage-misc-cachefunc to public/24292

[fchapoton]

comment:10

I removed the list. Otherwise looks good to me. Jeroen, do you approve ?

---

New commits:

6804e44	Merge branch 'u/embray/python3/sage-misc-cachefunc' in 8.2.b0
1a60988	trac 24292 some details

[fchapoton]

comment:11

green bot. Looks good to me.

[fchapoton]

Reviewer: Frédéric Chapoton, Jeroen Demeyer

[vbraun]

Changed branch from public/24292 to 1a60988