search

dancasimiro / WAV.jl

Julia package for working with WAV files
Other
85 stars 35 forks source link

Don't warn about WAVPlay replacement on reimport and wavplay function redefinition on compile #75

Closed nayyarv closed 5 years ago

nayyarv commented 5 years ago

As per #74,

You have runtime search for the library, but not a reimport if it already exists. This is kind of python inspired, so let me know if you'd prefer something else.

This should be reproducible where you create two packages with WAV as a dependency and try using them

julia> using PackageEg1
WARNING: replacing module WAVPlay

Moving the defaulting warning wavplay function to the __init__ fixes the precompile issue 

julia> using PackageEg2
[ Info: Precompiling PackageEg2 [top-level]
WARNING: Method definition wavplay(Any, Any) in module WAV at /home/nayyarv/.julia/packages/WAV/uORV0/src/WAV.jl:26 overwritten in module WAVPlay at /home/nayyarv/.julia/packages/WAV/uORV0/src/wavplay-pulse.jl:83.
WARNING: replacing module WAVPlay.

since it's never defined unless it needs to be (maybe this a thing you could do with older Julia)

I still believe that this should be done at compile time. What I think you could do, would be to do a check if libpulse is installed and is different and throw a warning suggesting a rebuild? That's what most other packages do in this case (from what I can tell)

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-2.0%) to 67.382% when pulling 1193ef78f84b2a8fda6448cadec9f30f8e266579 on nayyarv:master into e93bf0b0cbfe4393afca0b162b504c68613953c8 on dancasimiro:master.

dancasimiro commented 5 years ago

This looks interesting. I will try to take a closer look soon. Thanks for the contribution.

nayyarv commented 5 years ago

Thanks Dan. Any chance I could request another release, v1.0.2? The error message doesn't show up on any machine I've used develop on, but it returns with a vengeance on new machines haha! Much appreciated!

dancasimiro commented 5 years ago

I’ll try to release something this weekend. I’m sorry that I didn’t get to it last week.

dancasimiro commented 5 years ago

@nayyarv I released v1.0.2 this morning. It should get picked up soon.

nayyarv commented 5 years ago

Hey dan, sorry to keep bothering you, but it appears that the JuliaRegistry method has changed somewhat since your last release.

This hasn't been updated https://github.com/JuliaRegistries/General/blob/master/W/WAV/Versions.toml

And having a read of https://github.com/JuliaRegistries/Registrator.jl, I think you might need to call on the registry bot?

dancasimiro commented 5 years ago

You are correct @nayyarv. The release process has changed and I don’t have that new toml file.

I am traveling this week without a computer. I think that I managed to install the release bot correctly, but I can’t create the pull request until next week. Do you mind preparing a pull request with the required info? I think that I can merge it from my phone.

nayyarv commented 5 years ago

Certainly, I'll have a look, from what I can tell it's as simple as invoking the bot and it generates the necessary files. Lemme work it out and get something sent your way