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.

Sign up for GitHub

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

Jump to bottom

Add one for AbstractString #19548

Merged
merged 8 commits into from
Dec 31, 2016
Merged

Add one for AbstractString #19548

merged 8 commits into from
Dec 31, 2016

Conversation

bdeonovic
Copy link
Contributor

From discussion in #19536

@nalimilan
Copy link
Member

Could you add a test for the new method?

nalimilan
nalimilan reviewed Dec 9, 2016
View reviewed changes
base/strings/basic.jl
@@ -108,6 +108,8 @@ julia> "Hello " * "world"
(.*){T<:AbstractString}(v::Vector{T},s::AbstractString) = [i*s for i in v]
(.*){T<:AbstractString}(s::AbstractString,v::Vector{T}) = [s*i for i in v]

one(s::AbstractString) = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better: one{T <: AbstractString}(s::T) = convert(T, "")

@kshyatt kshyatt added the strings "Strings!" label Dec 10, 2016
@bdeonovic
Copy link
Contributor Author

Does adding a jldoctest constitute as adding a test? Or does it have to be added separately somewhere?

@nalimilan
Copy link
Member

I don't think you need to document the new method at all, since it matches the general docstring about one. So just add the test to test/strings/basic.jl.

@tkelman tkelman added the needs tests Unit tests are required for this change label Dec 11, 2016
nalimilan
nalimilan reviewed Dec 11, 2016
View reviewed changes
base/strings/basic.jl
@@ -108,6 +108,8 @@ julia> "Hello " * "world"
(.*){T<:AbstractString}(v::Vector{T},s::AbstractString) = [i*s for i in v]
(.*){T<:AbstractString}(s::AbstractString,v::Vector{T}) = [s*i for i in v]

one{U<:AbstractString}(::Type{U}) = convert(U, "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being a pain, but why use U when the rest of the file uses T?

@nalimilan
Copy link
Member

Still needs a test.

nalimilan
nalimilan approved these changes Dec 12, 2016
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bdeonovic
Copy link
Contributor Author

I did it! I contributed to open source =D

@nalimilan
Copy link
Member

Beware, once you start it's hard to stop...

@tkelman
Copy link
Contributor

tkelman commented Dec 12, 2016 •
edited
Loading

Thanks for the contribution @bdeonovic, but I still think this is a pun and more likely to obscure a bug in code that shouldn't be receiving strings than actually be useful for concatenation. Instead of prod, concatenating an array of strings would be better done as string(A...)

@bdeonovic
Copy link
Contributor Author

@tkelman I agree, personally I didn't want to include * as concatenation, but since it was there I felt that one was needed.

@oscardssmith
Copy link
Member

A recent issue about using sparse matrices with strings suggests that we should probably have zero also.

stevengj
stevengj reviewed Dec 12, 2016
View reviewed changes
base/strings/basic.jl
@@ -108,6 +108,8 @@ julia> "Hello " * "world"
(.*){T<:AbstractString}(v::Vector{T},s::AbstractString) = [i*s for i in v]
(.*){T<:AbstractString}(s::AbstractString,v::Vector{T}) = [s*i for i in v]

one{T<:AbstractString}(::Type{T}) = convert(T, "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, it should probably be:

one{T<:AbstractString}(::Union{T,Type{T}}) = convert(T, "")

in analogy with e.g. one(17) == 1.

@stevengj stevengj mentioned this pull request Dec 12, 2016
Closed
@stevengj
Copy link
Member

+1. As long as * is string concatenation, one should be the multiplicative identity.

@tkelman tkelman removed the needs tests Unit tests are required for this change label Dec 12, 2016
@Sacha0
Copy link
Member

Sacha0 commented Dec 12, 2016

As long as * is string concatenation, one should be the multiplicative identity.

On the one hand I agree with this. On the other I share @tkelman's operator punning sentiments: These issues seem to evidence the anti-punning position in e.g. #11030, rather than providing cause to double down on the pun. Best!

@StefanKarpinski
Copy link
Member

We added the ++ operator so that it could be used for string concatenation, but that never caught on, so we're still using * it seems. If someone wants to take on the whole ++ change (or something else entirely), feel free, but until then, this makes sense.

@ararslan
Copy link
Member

Bump! Will this make it into 0.6?

@stevengj
Copy link
Member

Yes, I think the consensus was clearly for this as long as * is concatenation.

@stevengj stevengj merged commit 6484f4c into JuliaLang:master Dec 31, 2016
@andreasnoack andreasnoack mentioned this pull request Jan 10, 2017
Closed
@oxinabox oxinabox mentioned this pull request Jan 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevengj stevengj stevengj left review comments

@nalimilan nalimilan nalimilan approved these changes

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees
No one assigned
Labels
strings "Strings!"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@bdeonovic @nalimilan @tkelman @oscardssmith @stevengj @Sacha0 @StefanKarpinski @ararslan @kshyatt