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

Conversation

6 participants
@bkamins
Copy link
Contributor

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.

@bkamins

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

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

@nalimilan

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

But they already are?

@bkamins

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

Sigh.

@bkamins

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

Any decision on this change?

@KristofferC KristofferC merged commit 9eda36f into JuliaLang:master Nov 13, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@bkamins bkamins deleted the bkamins:char_iterator_size branch Nov 13, 2018

@bkamins

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2018

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

@StefanKarpinski

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

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 added a commit to tkf/julia that referenced this pull request Nov 21, 2018

@fredrikekre fredrikekre removed the needs news label Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.