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:

Status badges

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 chapoton

  • Branch set to u/chapoton/27443
  • Commit set to 1303a1ddf1f064c95b8072791e28677496a12383
  • Status changed from new to needs_review

New commits:

1303a1dspring cleanup of QQbar (pep8, removing deprecated stuff)

comment:2 Changed 3 years ago by chapoton

  • Cc tscrim added

green bot, please review

comment:3 Changed 3 years ago by tscrim

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 chapoton

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 tscrim

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 git

  • Commit changed from 1303a1ddf1f064c95b8072791e28677496a12383 to d24560a7d16fbc8022db6b9d3543ccd8beb50289

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

d24560atrac 27443 change-back the extras parameter of completions in QQbar

comment:7 Changed 3 years ago by chapoton

done, back to {}

comment:8 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you. LGTM.

comment:9 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/27443 to d24560a7d16fbc8022db6b9d3543ccd8beb50289
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.