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.

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

fixed deprecation warnings thrown by julia 0.4. Did NOT use the Compat ... #99

Merged
merged 1 commit into from
Feb 14, 2015

Conversation

Cody-G
Copy link

@Cody-G Cody-G commented Feb 4, 2015

...package to ensure compatibility with earlier Julia versions.

@johnmyleswhite
Copy link
Contributor

Thanks for doing this, Cody. Let's plan to merge this when 0.4 becomes a release candidate.

@mlubin
Copy link
Contributor

mlubin commented Feb 4, 2015

What's the reason for not using Compat? We won't be able to merge this for months.

@Cody-G
Copy link
Author

Cody-G commented Feb 4, 2015

I'm new to the scene (I'm a student in Tim's lab, that was my first pull
request ever). So if I'm not following the standard procedure, please do
tell me. I didn't use Compat because I wasn't sure whether the lines were
incompatible with previous versions of Julia or not, and I had imagined
that a lot of unnecessary @compat statements would clutter the code. Is it
better practice to use @compat even when I'm not sure? Thanks for your
patience.

On Wed, Feb 4, 2015 at 9:15 AM, Miles Lubin [email protected]
wrote:

What's the reason for not using Compat? We won't be able to merge this for
months.


Reply to this email directly or view it on GitHub
#99 (comment).

@mlubin
Copy link
Contributor

mlubin commented Feb 5, 2015

@Cody-G, I apologize for the unduly alarmist tone. In a publicly released package, it's a good idea to keep the master branch (or equivalent) compatible with the latest julia release unless there's a good reason (like a new feature that's needed). Otherwise it makes it difficult to provide updates to users on julia 0.3 and 0.4 at the same time. So the @compat statements are a bit ugly, but they make it much easier to maintain the code.
The little red X next to your commit means that it failed the Travis CI tests, and you can see that it worked on 0.4 but not 0.3. If you open a new PR (or update this one) you can use this to double check that your changes haven't broken anything.

@timholy
Copy link
Contributor

timholy commented Feb 5, 2015

Hey, Cody, thanks for doing this!

Others, note this is not against master, it's against teh/constrained. Still, in case anyone besides us is using this, it's probably best not to break 0.3 support. Fortunately, I think you'll only need a @compat in front of the Dict statements, I think the rest are valid syntax on 0.3 or are provided by Compat as plain functions.

@Cody-G
Copy link
Author

Cody-G commented Feb 14, 2015

I appreciate the feedback, and I'm sorry for the delay. I've added @compat statements where appropriate, but the Travid CI tests now fail becuase the Compat package was not found. Is it necessary to change a .yml file to have Travis install package dependencies before testing? I've never used Travis before. Thanks again!

@timholy
Copy link
Contributor

timholy commented Feb 14, 2015

Nice. I think all you'd need to do to fix this is to add Compat to Optim's REQUIRE file, and the package manager will install it automatically.

…age to ensure compatibility with earlier Julia versions.
@Cody-G
Copy link
Author

Cody-G commented Feb 14, 2015

Thanks, I squashed and pushed and it's passing now. Travis is nice!

@timholy
Copy link
Contributor

timholy commented Feb 14, 2015

Yep, keeps us from making all sorts of mistakes. It's amazing how many bugs only happen on someone else's machine 😄.

timholy added a commit that referenced this pull request Feb 14, 2015
fixed deprecation warnings thrown by julia 0.4.  Did NOT use the Compat ...
@timholy timholy merged commit 7ed8c2a into JuliaNLSolvers:teh/constrained Feb 14, 2015
@Cody-G Cody-G deleted the fix_deprecations branch February 15, 2015 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants