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 RepString from Base #9

Merged
merged 3 commits into from
Dec 15, 2016
Merged

Add RepString from Base #9

merged 3 commits into from
Dec 15, 2016

Conversation

ararslan
Copy link
Member

tkelman
tkelman reviewed Dec 13, 2016
View reviewed changes
src/LegacyStrings.jl
@@ -81,4 +82,10 @@ import Base:
else
using Base: UTF_ERR_SHORT, checkstring
end

if VERSION >= v"0.6.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should use isdefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, thanks!

@ararslan
Copy link
Member Author

The failure on 32-bit Windows nightly is a timeout.

@StefanKarpinski
Copy link
Member

LGTM – I assume you just copied the code from Base + the relevant tests?

@ararslan
Copy link
Member Author

assume you just copied the code from Base + the relevant tests?

Yep. I didn't copy the definition of repeat that returns a RepString though. Should I?

@stevengj
Copy link
Member

LGTM.

@stevengj
Copy link
Member

Are there docstrings that need to be copied over?

@stevengj
Copy link
Member

Also, the PR should update README.md

@ararslan
Copy link
Member Author

ararslan commented Dec 14, 2016 •
edited
Loading

Are there docstrings that need to be copied over?

Nope.

Also, the PR should update README.md

Good call, will do. Thanks!

Edit: Done.

@ararslan ararslan merged commit 96c851f into master Dec 15, 2016
@ararslan ararslan deleted the aa/repstring branch December 15, 2016 19:23
@ararslan
Copy link
Member Author

ararslan commented Jan 4, 2017

I just realized that I forgot to make the corresponding Base PR that removes RepString. Now that this has been merged and the package has been tagged to include it, I'll do that.

@tkelman
Copy link
Collaborator

tkelman commented Jan 4, 2017

Jeff beat you to it JuliaLang/julia#19867 (which is why I went and tagged this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

@StefanKarpinski StefanKarpinski Awaiting requested review from StefanKarpinski

@stevengj stevengj Awaiting requested review from stevengj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ararslan @StefanKarpinski @stevengj @tkelman