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

Add IteratorSize for Char #29819

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 26, 2018

See discussion in https://discourse.julialang.org/t/broadcasting-and-single-characters/16836.
I am not sure if the current behavior is indented, or what I propose in this PR is intended.

This can be classified as breaking change or bug fix - I am not sure what is the policy for such PRs after Julia 1.0 release.

@KristofferC KristofferC added the minor change Marginal behavior change acceptable for a minor release label Oct 26, 2018
@bkamins
Copy link
Member Author

bkamins commented Oct 27, 2018

CI fails in one case and it seems unrelated (but I am not 100% sure, as the place of the error is strange and it should not happen).

@StefanKarpinski
Copy link
Member

Seems reasonable to me. Does anyone with strong opinions about broadcasting want to chime in? @mbauman, @JeffBezanson?

@nalimilan
Copy link
Member

nalimilan commented Oct 27, 2018

+1 for fixing broadcasting, but maybe Chars should remain HasLength (like strings and symbols) rather than HasShape{0} (like numbers)? AFAICT making them HasShape{0} would change the result of [x for x in 'a'] from a one-element vector to a zero-dimensional array. If you only define broadcastable(c::Char) = c you should fix broadcasting without changing comprehensions.

@bkamins
Copy link
Member Author

bkamins commented Oct 27, 2018

Agreed this is a change - that is why I have said that I am not sure what is the intended behavior.

I understood that the general design intention was that [x for x in 'a'] and [x for x in 1] should behave the same (both producing 0-dimensional array).

Also I think that we should have that identity.('a') and [x for x in 'a'] produce the same thing (what would be the reason for a different behavior)?

@StefanKarpinski
Copy link
Member

I really don’t want to make characters iterable. We wanted to make numbers non-iterable but it was too disruptive so let’s not make another scalar type iterable for no reason.

@KristofferC
Copy link
Member

But they already are?

@bkamins
Copy link
Member Author

bkamins commented Oct 29, 2018

My understanding is that "we are", but only half-way. This PR makes Char consistent with numbers.

I would also prefer Char to be treated purely as a scalar, but this would be even more breaking.

@mbauman
Copy link
Member

mbauman commented Oct 29, 2018

Given that Chars are already iterable and already define size(c::AbstractChar) = () and axes similarly, adding this trait simply brings everything into sync.

@StefanKarpinski
Copy link
Member

Sigh.

@bkamins
Copy link
Member Author

bkamins commented Nov 13, 2018

Any decision on this change?

@KristofferC KristofferC merged commit 9eda36f into JuliaLang:master Nov 13, 2018
@KristofferC KristofferC added the needs news A NEWS entry is required for this change label Nov 13, 2018
@bkamins bkamins deleted the char_iterator_size branch November 13, 2018 15:53
@bkamins
Copy link
Member Author

bkamins commented Nov 13, 2018

@KristofferC Shall I make a PR to news, or this is now done in some other way?

@StefanKarpinski
Copy link
Member

Yes, PR to add an entry to that file would be perfect. Thank you!

KristofferC added a commit that referenced this pull request Nov 13, 2018
tkf pushed a commit to tkf/julia that referenced this pull request Nov 21, 2018
@fredrikekre fredrikekre removed the needs news A NEWS entry is required for this change label Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants