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

Package breaks 0n 0.5 with a " invalid iteration specification" #17668

Closed
joa-quim opened this issue Jul 28, 2016 · 16 comments
Closed

Package breaks 0n 0.5 with a " invalid iteration specification" #17668

joa-quim opened this issue Jul 28, 2016 · 16 comments
Assignees
Labels
docs This change adds or pertains to documentation regression Regression in behavior compared to a previous version

Comments

@joa-quim
Copy link

Hi,

This is the GMT package that works fine with 0.4 and used to work too with a some months old 0.5dev, but now it errors on loading with

using GMT
... (deprecation messages)
WARNING: Base.UTF8String is deprecated, use String instead.
  likely near C:\j\.julia\v0.5\GMT\src\gmt_main.jl:57
ERROR: LoadError: LoadError: syntax: invalid iteration specification
 in include_from_node1(::String) at .\loading.jl:426 (repeats 2 times)
 in eval(::Module, ::Any) at .\boot.jl:234
 in require(::Symbol) at .\loading.jl:357
while loading C:\j\.julia\v0.5\GMT\src\gmt_main.jl, in expression starting on line 149
while loading C:\j\.julia\v0.5\GMT\src\GMT.jl, in expression starting on line 49

The offending code is

    XX = Array(GMT_RESOURCE, 1, n_items)
    for (k = 1:n_items)
        XX[k] = unsafe_load(X, k) 
    end

Julia version (nightly)

Version 0.5.0-rc0+4 (2016-07-27 09:20 UTC)
Commit 347057b (1 day old master)
x86_64-w64-mingw32

@stevengj
Copy link
Member

It should be for k = 1:n_items. This was always the recommended style, but you are right that the parentheses were accepted under 0.4 but are rejected under 0.5.

@JeffBezanson, do you know what PR changed this behavior? It should be listed under "breaking changes" in the NEWS in any case.

@stevengj stevengj added the docs This change adds or pertains to documentation label Jul 28, 2016
@joa-quim
Copy link
Author

I resisted years to Matlab trying to convince me to drop the parentheses. Call it old fashion but I like wrapping fors and ifs with parentheses.

What is the need to do such a needless breaking compatibility?

@stevengj
Copy link
Member

The parentheses are C syntax. It's usually a bad idea to force the syntax from one language on another. When in Rome....

@JeffBezanson
Copy link
Member

Changed by 8d76566, but this aspect of the change was not intentional.

@StefanKarpinski
Copy link
Member

I feel fairly strongly that the iterator part of a for loop should not be allowed to be parenthesized. The implicit theory behind allowing that is that the k = 1:n part of for k = 1:n is just an expression, which it isn't – it's part of the for syntax. For if statements it's a different story – the condition is just an expression so you can parenthesize it or not as you want.

@tkelman tkelman added the regression Regression in behavior compared to a previous version label Jul 28, 2016
@joa-quim
Copy link
Author

And I feel sad that an ages old principle of "use parentheses freely to increase readability" has been dropped in favor of a ... I don't know what.

@Keno
Copy link
Member

Keno commented Jul 29, 2016

While I don't have strong feelings one way or another, I agree with Stefan that this situation is different from allowing parentheses around expressions. It is however the same as a binding in let, so it needs to be consistent with:

julia> let (k = 1)
       k
       end
1

@JeffBezanson
Copy link
Member

an ages old principle of "use parentheses freely to increase readability"

Not in Lisp, one of the oldest programming languages :)

@JeffBezanson
Copy link
Member

I also don't have strong feelings about this, but I will emphasize again that this was not a deliberate decision, so it's a bit overwrought to talk of dropping age-old principles or whatnot.

@tkelman
Copy link
Contributor

tkelman commented Jul 29, 2016

We usually do try to go through deprecation periods for as many of these things as we can, when they are intentional decisions (like f (x) for example)

@joa-quim
Copy link
Author

"Not in Lisp, one of the oldest programming languages :)"

And who said Lisp is readable anyway? :)

Sorry if it sound a bit hash and yes I understand that it was unintentional but from Stephan's reply it's pretty much looks like that it's here to stay.

@tkelman
Copy link
Contributor

tkelman commented Jul 30, 2016

If we can change it from an error to a deprecation (or maybe allow it to continue to work?) then I think we should.

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2016

Jeff, can this be fixed or turned into a deprecation warning, or should it be added to NEWS as an accidental breaking change?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Aug 1, 2016

There is no ancient principle of "stick parens wherever you like, it will mean the same thing". What there was and still is is the fact that parenthesizing a complete expression does not change it's meaning. The iteration part of a for loop is not an expression, which was precisely my point.

We should:

  • Either:
    1. Add a deprecation warning or clearer error message for this syntax change, and
    2. Also deprecate / error for parens in let, global, local, etc.
  • Or:
    1. Make optional parens an official part of the syntax for these constructs, and
    2. Support them in for, let, global, local, etc. consistently.

I'm in favor of the former option – we should have fewer pointless syntax variations not more.

@JeffBezanson
Copy link
Member

I'd be happy to add a usual deprecation for this.

@JeffBezanson JeffBezanson self-assigned this Aug 1, 2016
@tkelman
Copy link
Contributor

tkelman commented Aug 2, 2016

Also deprecate / error for parens in let, global, local, etc.

We should probably revisit these later and make them consistent too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

6 participants