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

Normalize all = to in in for loops #70

Closed
wants to merge 2 commits into from
Closed

Normalize all = to in in for loops #70

wants to merge 2 commits into from

Conversation

odow
Copy link
Contributor

@odow odow commented Sep 16, 2019

Since #34 was closed, there has been a steady stream of people disagreeing. I get that it is a contentious issue with no settled style in the community, but this simplifies the rule:

  • Use for i in iter except it iter is a literal range, then use for i = 1:n.

to

  • Always use for i in iter.

Revises #35. Re-closes the already closed issue: #34

@StefanKarpinski
Copy link
Contributor

StefanKarpinski commented Sep 16, 2019

How do you propose catching the common issue where people write for i = n when they meant for i = 1:n? Also, if you grep through the Julia stdlib code, this is directly against the common practice. I suspect that is also true in much of the rest of the ecosystem. I get that you want simple rules, but I think this change is going to make acceptance of JuliaFormatter much harder to push.

@StefanKarpinski
Copy link
Contributor

Looking through more cases, Base + stdlib Julia code is actually remarkably consistent about following the rule that is currently implemented.

@odow
Copy link
Contributor Author

odow commented Sep 16, 2019

How do you propose catching the common issue where people write for i = n when they meant for i = 1:n?

Do you mean from a correctness standpoint? Or a stylistic standpoint?

julia> function foo(n)
           for i in n
               println(i)
           end
       end
foo (generic function with 1 method)

julia> function bar(n)
           for i = n
               println(i)
           end
       end
bar (generic function with 1 method)

julia> foo(5)
5

julia> bar(5)
5

@StefanKarpinski
Copy link
Contributor

StefanKarpinski commented Sep 16, 2019

Yes, I mean from a correctness standpoint. If the convention is that one uses in with an iterator and = with a literal range, then you have an additional bit of information about user intent. That bit is redundant, but it acts as a "parity bit" and the user gets a warning when the parity bit doesn't match the usage. For example if the user writes for i = n then the = doesn't match the nature of the n which is not a literal range, so the linter or formatter can call their attention to the mismatch. You are proposing stylistically disallowing the usage of that parity bit to help convey intent.

@odow
Copy link
Contributor Author

odow commented Sep 16, 2019

Okay, fair point. I see the benefit, and I've lost some zeal for this change.

To summarize: the trade off is therefore between people making the mistake for i = n and using a linter to find catch this error, and making everyone writing Julia learns when to use the difference between = and in. Against standardizing on in and having more incorrect code.

I'm not sure which I prefer...

I guess the root issue is that numbers are iterable. (Slightly off-topic, but was there a reason for this?)

@mlubin
Copy link

mlubin commented Sep 16, 2019

For example if the user writes for i = n then the = doesn't match the nature of the n which is not a literal range, so the linter or formatter can call their attention to the mismatch.

FWIW, given a good auto-formatter, the default workflow for me would be "auto format and forget", which means that if I wrote "for i = n", it would just get corrected without raising a flag to "for i in n" regardless of if I intended to write a literal range. In this case the information in the "parity bit" is quickly lost.

It would be another question if the proposal was to make for i = n a syntax error which always requires user intervention to fix.

@StefanKarpinski
Copy link
Contributor

Agree, it's probably not very effective just as part of the standard formatting. To be effective, it needs to also be something that linters can warn about, but for that to be possible, it needs to be standard. Even in situation where one blindly runs the formatter and later goes bug hunting, at least the fact that it's for i in n gives more visual clue that the RHS isn't the intended range.

I guess the root issue is that numbers are iterable. (Slightly off-topic, but was there a reason for this?)

JuliaLang/julia#7903

@domluna
Copy link
Owner

domluna commented Sep 16, 2019

I think generally = is used for literal ranges apart from repos which strictly follow https://github.com/jrevels/YASGuide or http://www.juliaopt.org/JuMP.jl/dev/style/.

At first I was in the use in for everything camp as well but the describing the intent and possibly catching an error as @StefanKarpinski mentioned above has grown on me.

Thoughts on allowing use of either = or in for a range? So these would both be correct:

for i = 1:10
end

for i in 1:10
end

But this would be incorrect:

for i = I
end

I believe this still captures the initiative for #34 (I might be wrong) and keeps the in folks happyish.

@mlubin
Copy link

mlubin commented Sep 19, 2019

@domluna I would prefer that you chose exactly one of the two options and make it a rule. We shouldn't carve out exceptions for special interest groups :)

@domluna
Copy link
Owner

domluna commented Sep 19, 2019

Oh man, "with great power comes great responsibility" a wise man once said.

We're gonna stick to the current implementation then. @odow I appreciate the effort ❤️

@domluna domluna closed this Sep 19, 2019
@odow odow deleted the od/in branch September 19, 2019 01:20
@domluna domluna mentioned this pull request Oct 5, 2019
2 tasks
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