Opened 10 years ago
Last modified 9 years ago
#13446 closed enhancement
Revert fix (but not doctests) from #11919 — at Version 6
| Reported by: | mjo | Owned by: | burcin |
|---|---|---|---|
| Priority: | major | Milestone: | sage-5.6 |
| Component: | symbolics | Keywords: | |
| Cc: | Merged in: | ||
| Authors: | Michael Orlitzky | Reviewers: | Burcin Erocal, Karl-Dieter Crisman |
| Report Upstream: | N/A | Work issues: | |
| Branch: | Commit: | ||
| Dependencies: | Stopgaps: |
Status badges
Description (last modified by )
Change History (7)
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
Is it a big deal? We at least need the code explaining why the doctest is there, but the explanation that's there now isn't really correct anymore. It's only a few lines and it serves to give anyone who would modify that function a little background on it.
Plus, that function doesn't wind up in the reference manual anyway, so it doesn't hurt anyone who isn't digging into the code.
comment:4 Changed 10 years ago by
I'm somewhat surprised to find myself on the side of less historical explanation, but I agree that this is a little confusing when reading the code directly. If anything, I'd remove text so that one just tests whether pickling works - which, to be honest, we do quite a bit of in Sage, so it shouldn't need any explanation. But this is probably all a bit of bikeshedding...
comment:5 Changed 10 years ago by
Ok, the second patch leaves the comment alone. The reviewer can thus choose his favorite version =)
comment:6 Changed 10 years ago by
- Description modified (diff)
- Reviewers set to Burcin Erocal, Karl-Dieter Crisman
- Status changed from needs_review to positive_review
Positive review to second version.
Do you really need to change the text before the doctest? IMHO, we don't need to discuss the history of the bug in the reference manual.