Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate sage.media.wav, since it is totally undoctested, and evidently full of confusing bugs #12673

Closed
williamstein opened this issue Mar 15, 2012 · 20 comments
Closed

Deprecate sage.media.wav, since it is totally undoctested, and evidently full of confusing bugs #12673

williamstein opened this issue Mar 15, 2012 · 20 comments
Assignees
@jasongrout
Labels
p: major / 3 t: bug
Milestone
sage-9.5

Comments

@williamstein
Copy link
Contributor

As I think http://480.sagenb.org/home/pub/137/ amply illustrates, the mere existence of the untested and very buggy module sage.media.wav causes more confusion than it is worth for now. It would be best to remove it from Sage, and only add something back as a result of a new (probably student) project to do things right.

CC: @jasongrout @sagetrac-mhampton

Component: misc

Author: Matthias Koeppe

Branch/Commit: 2bf2bd0

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/12673
@williamstein williamstein added this to the sage-5.11 milestone Mar 15, 2012
@williamstein
Copy link
Contributor Author

Attachment: trac_12673.patch.gz

@williamstein
Copy link
Contributor Author

comment:2

Alternatively, doctest the code. I've attached a patch to delete it, but maybe that is too draconian.

@williamstein williamstein changed the title remove sage.media.wav, since it is totally undoctested, and evidently full of confusing bugs fix sage.media.wav, since it is totally undoctested, and evidently full of confusing bugs Mar 15, 2012
@kcrisman
Copy link
Member

comment:3

This is (very tangentially) related to #7668 and this interact by Marshall.

Given that it's not the worst to eventually have easy-to-use wave handling, perhaps it would be appropriate to split the difference between doctesting and deleting. Could one simply comment out all the code and raise (doctested?) NotImplementedErrors for everything here?

@jdemeyer
Copy link

comment:4

Please fill in your real name as Author.

@kcrisman
Copy link
Member

kcrisman commented May 8, 2013

comment:5

I've changed my mind. I think that we should just replace this by some really nice documentation about how to use the wave module, given this sage-devel discussion and this ask.sagemath question. What would be appropriate to roughly replace this stuff with?

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@mkoeppe mkoeppe changed the title fix sage.media.wav, since it is totally undoctested, and evidently full of confusing bugs Deprecate sage.media.wav, since it is totally undoctested, and evidently full of confusing bugs Aug 26, 2021
@mkoeppe mkoeppe modified the milestones: sage-6.4, sage-9.5 Aug 26, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 26, 2021

Branch: u/mkoeppe/deprecate_sage_media_wav__since_it_is_totally_undoctested__and_evidently_full__of_confusing_bugs

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 26, 2021

New commits:

2db5da6Deprecate sage.media

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 26, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 26, 2021

Commit: 2db5da6

@tscrim
Copy link
Collaborator

tscrim commented Aug 26, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 26, 2021

comment:13

LGTM.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 27, 2021

comment:14

Thanks!

@kcrisman
Copy link
Member

comment:15

Are there no doctests at all that will get confused here? Maybe we should add one along the lines of the ones in sage.finance.

@kcrisman
Copy link
Member

comment:16

(I mean #32427.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2021

Changed commit from 2db5da6 to 2bf2bd0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2021

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

2bf2bd0src/sage/media/wav.py: Add deprecation to doc

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 27, 2021

comment:18

There was not a single doctest. Now there is one.

@kcrisman
Copy link
Member

comment:20

There was not a single doctest. Now there is one.

It was definitely the wild wild west in those days.

Thanks, and sorry to override Travis' review :) I will assume patchbot will be happy and restore his assessment.

@tscrim
Copy link
Collaborator

tscrim commented Aug 27, 2021

comment:21

Replying to @kcrisman:

Thanks, and sorry to override Travis' review :) I will assume patchbot will be happy and restore his assessment.

Don't worry about it at all. We are a community with different options, and we as a collective should agree about positive reviews.

@vbraun
Copy link
Member

vbraun commented Aug 31, 2021

Changed branch from u/mkoeppe/deprecate_sage_media_wav__since_it_is_totally_undoctested__and_evidently_full__of_confusing_bugs to 2bf2bd0

@vbraun vbraun closed this as completed in c326905 Aug 31, 2021
@williamstein williamstein mentioned this issue Feb 22, 2013
Closed
mkoeppe added a commit to mkoeppe/sage that referenced this issue Sep 19, 2023
@mkoeppe
sage.media, sage.structure.graphics_file: Remove; deprecated in sagem…
3f2a1d5 
…ath#12673 (2021), sagemath#32988 (2022)
vbraun pushed a commit to vbraun/sage that referenced this issue Sep 23, 2023
sagemathgh-36305: sage.media, sage.structure.graphics_file: Remov…
d7404b1 
…e; deprecated in sagemath#12673, sagemath#32988

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36305
Reported by: Matthias Köppe
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jasongrout jasongrout

Labels
p: major / 3 t: bug
Projects
None yet
Milestone
sage-9.5
Development

No branches or pull requests
7 participants
@jasongrout @vbraun @williamstein @tscrim @kcrisman @jdemeyer @mkoeppe