Opened 3 years ago
Closed 3 years ago
#27443 closed enhancement (fixed)
spring cleanup for QQbar
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.7 |
Component: | algebra | Keywords: | |
Cc: | tscrim | Merged in: | |
Authors: | Frédéric Chapoton | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | d24560a (Commits, GitHub, GitLab) | Commit: | d24560a7d16fbc8022db6b9d3543ccd8beb50289 |
Dependencies: | Stopgaps: |
Description
removing one deprecated class (after #19954)
also fix most of the pep8 warnings
plus a few minor code improvements
Change History (9)
comment:1 Changed 3 years ago by
- Branch set to u/chapoton/27443
- Commit set to 1303a1ddf1f064c95b8072791e28677496a12383
- Status changed from new to needs_review
comment:3 Changed 3 years ago by
I do not understand the extras=None
change. I think it is fine to have an optional parameter that has a default value as optional is a way to convey a little more context/information to the user (rather than saying it is a default value). If there is something that is passing None
, then I feel that should be changed.
comment:4 Changed 3 years ago by
The reason is the following (as explained by lgtm):
When a parameter default value is a dictionary, it is stored in the function definition, and any modification of this dictionary is acting on (changing) this stored default. So the next call of the function will use the modified default. Which is not the expected effect.
comment:5 Changed 3 years ago by
I was not aware of that, but it makes sense that things were designed that way. That sure can lead to subtle bugs.
sage: def foo(x, d={}): ....: d[x] = 1 ....: return d ....: sage: x = foo(5) sage: x {5: 1} sage: x[10] = 10 sage: foo(0) {0: 1, 5: 1, 10: 10}
However, in these cases the dict
is only expanded and never changed. So I don't see the point of making the code more complex to silence a warning:
sage: def bar(**opts): ....: opts[1] = 5 ....: sage: def foo(d={}): ....: bar(**d) ....: return d ....: sage: foo() {} sage: foo() {}
comment:6 Changed 3 years ago by
- Commit changed from 1303a1ddf1f064c95b8072791e28677496a12383 to d24560a7d16fbc8022db6b9d3543ccd8beb50289
Branch pushed to git repo; I updated commit sha1. New commits:
d24560a | trac 27443 change-back the extras parameter of completions in QQbar
|
comment:7 Changed 3 years ago by
done, back to {}
comment:8 Changed 3 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thank you. LGTM.
comment:9 Changed 3 years ago by
- Branch changed from u/chapoton/27443 to d24560a7d16fbc8022db6b9d3543ccd8beb50289
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
spring cleanup of QQbar (pep8, removing deprecated stuff)