Fixes for Julia 0.7 #62
Conversation
src/WAV.jl
Outdated
| using FileIO | ||
|
|
||
| function __init__() | ||
| module_dir = dirname(@__FILE__) | ||
| if Libdl.find_library(["libpulse-simple"]) != "" | ||
| if Libdl.find_library(["libpulse-simple.so.0"]) != "" |
There was a problem hiding this comment.
Are there systems where this does not work? I.e. should this be Libdl.find_library(["libpulse-simple.so.0", "libpulse-simple.so"]) or similar?
There was a problem hiding this comment.
I don’t think that this works on Mac OS X, where the shared object extension is dylib.
There was a problem hiding this comment.
Ah, I wasn't aware this was used on OSX, too. Does it still work unversioned there? Or would it have to be "libpulse-simple.0.dylib" there? Anyway, I can definitely put the unversioned entry back, than at least it won't break anything.
There was a problem hiding this comment.
I have not tested recently, but the the bare string used to work on Linux and OS X. I expect it to work on Windows as well, but I have never tested on that operating system.
There was a problem hiding this comment.
The unversioned string fails on Linux since JuliaLang/julia#22828 (0.7.0-DEV.3588).
There was a problem hiding this comment.
That’s too bad. I suppose that I could add checks for each expected file extension.
src/WAV.jl
Outdated
| @@ -517,7 +519,7 @@ function read_data(io::IO, chunk_size, fmt::WAVFormat, format, subrange) | |||
| # "format" is the format of values, while "fmt" is the WAV file level format | |||
| convert_to_double = x -> convert(Array{Float64}, x) | |||
|
|
|||
| if subrange === Void | |||
| if subrange === nothing || subrange === Nothing # Nothing kept for legacy reasons only | |||
There was a problem hiding this comment.
I've changed the default value from the type Void (now Nothing) to its singleton value nothing, which is much more idiomatic. In case anyone explicitly used subrange=Void, this will be handled here. Should there be a depwarn instead of silently accepting?
There was a problem hiding this comment.
I think that you can break the API here. There is another breaking change on master that will be released with Julia 1.0.
There was a problem hiding this comment.
I think that you can break the API here.
Ok. With a deprecation or hard-breaking? Also, on second thought, subrange=: might be even better than subrange=nothing. Preferences?
There is another breaking change on master that will be released with Julia 1.0.
But you do plan on releasing a version that works with Julia 0.7 before 1.0 comes out, right?
There was a problem hiding this comment.
Yeah, I do plan to make a release that supports both. Do the Julia deve still plan to make 0.7 api compatible with 1.0? I haven’t been following as closely as I once did.
There was a problem hiding this comment.
Yes, 1.0 should only drop functionality that's already deprecated in 0.7. So with this PR, WAV.jl should be ready for Julia 1.0 (fingers crossed).
Meanwhile, what's your opinion on subrange=: vs. subrange=nothing and deprecation yes/no?
There was a problem hiding this comment.
I prefer subrange=:. You can add a deprecation if it isn't too much of a hassle. I would keep the deprecation for the Julia 0.7 release cycle and remove it when Julia 1.0 ships.
| # Get `subchunk2size`. | ||
| seek(io,40) | ||
| # Get `subchunk2size`. | ||
| seek(io, 24 + subchunk_size) |
There was a problem hiding this comment.
This seems totally unrelated to the migration from Julia 0.6 to 0.7. How did this ever work with seeking to 40? Usually the position should be 64.
There was a problem hiding this comment.
This was submitted recently. I think that there is a unit test that goes with it.
There was a problem hiding this comment.
See pull request #61. It used to have a value of 64.
There was a problem hiding this comment.
Ah, yes, without this change, I was seeing the same test failures as in the CI log of that PR:
Test Failed
Expression: length(y) == 2 * length(x)
Evaluated: 8000 == 16000
ERROR: LoadError: There was an error during testing
while loading /home/travis/.julia/v0.6/WAV/test/runtests.jl, in expression starting on line 45
So apparently, #61 fixed a bug and added a test for it, but broke another test. With this update, they both pass. Without investigating closer, I guess the two tests append to WAV files with fmt chunks of different lengths, so a fixed value, 40 or 64, cannot be right for both.
src/WAVChunk.jl
Outdated
| @@ -1,15 +1,15 @@ | |||
| """ | |||
| A RIFF chunk. | |||
| """ | |||
| type WAVChunk | |||
| mutable struct WAVChunk | |||
There was a problem hiding this comment.
This could probably be immutable.
There was a problem hiding this comment.
Is there any code that modifies the type?
There was a problem hiding this comment.
Not within WAV.jl, but I don't know what the intended uses outside WAV.jl (or inside it in the future) might be.
There was a problem hiding this comment.
I need to check, but I believe that this type is new. I think that it came with #61 as well, which has not been part of a release.
There was a problem hiding this comment.
Came with #56, which is also not yet part of a release. So making it immutable now should be safe. If the need arises in the future, one can always make it mutable, while the opposite direction would be breaking. Should I use this opportunity then to make it immutable?
There was a problem hiding this comment.
Yes, please make the type immutable.
martinholters
force-pushed
the
mh/fixes_0.7
branch
from
99a2658
to
5b0f0e6
Compare
And use `:` instead of `Nothing` as default value for `subrange`.
Make WAVChunk immutable while at it.
Replace `Display`, `reprmime`, and `ismatch` with `AbstractDisplay`, `repr`, and `occursin`, respectively.
martinholters
force-pushed
the
mh/fixes_0.7
branch
from
5b0f0e6
to
53addc4
Compare
This should fix (most of) the deprecations on Julia 0.7 and make it usable there again. I've only tried
wavplayon Linux, though, so no idea whether audioqueue works. Some more remarks follow inline.The tests still show a bunch of deprecation warnings for implicit assignment to global variables. From a cursory glance, these should be safe to ignore because the assigned values are not used in global scope. Best way to get rid of the warnings would probably be wrapping things in testsets (to introduce explicit local scopes).