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 documentation of SubString #23957

Merged
merged 3 commits into from
Oct 3, 2017
Merged

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 1, 2017

SubString first appears in the section of regular expressions, but it is not mentioned earlier. I suggest to add documentation of SubString as it is returned by several functions from the standard library.

`SubString` first appears in the section of regular expressions, but it is not mentioned earlier. I suggest to add documentation of `SubString` as it is returned by several functions from the standard library.
@kshyatt kshyatt added docs This change adds or pertains to documentation strings "Strings!" labels Oct 2, 2017
Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

I was going to suggest adding a @ref to a SubString but it seems there is no corresponding docstring for SubString.

@nalimilan
Copy link
Member

Thanks! I find it more natural to add this description after the presentation of getindex, at the end of "String Basics", don't you? The end of the section is already a good transition since it explains that chars and strings are very different. You could start saying that indexing creates a copy by default, but that there's a way to avoid it.

@bkamins
Copy link
Member Author

bkamins commented Oct 2, 2017

@nalimilan moved, I agree that it is better placed here.

SubString{String}
```

`SubString` works like [`getindex`](@ref), but it does not make a copy of the parent string. Several
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, one problem is that getindex isn't presented for strings. Maybe just say that indexing in the examples presented above make a copy, but that one can also use SubString. Also, it would make more sense IMHO to say this before the examples: else, it's not clear what a "view" is nor why one would want to create a view.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what I also thought after committing - I hope the current wording is clear. Sorry for the confusion.

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, looks good! If you want to add a docstring for SubString in another PR, you're welcome!

@bkamins
Copy link
Member Author

bkamins commented Oct 2, 2017

I would add SubString documentation after #22511 is merged.

@rfourquet
Copy link
Member

As String are (supposed to be) immutable, I was wondering whether it would make sense to remove the SubString type altogether, and handle copy vs substring at a lower level (in C?), invisible for the user?

@bkamins
Copy link
Member Author

bkamins commented Oct 2, 2017

I am not an expert on C internals of Julia, but SubString can accept any AbstractString so I believe it could be difficult in general.

@fredrikekre fredrikekre merged commit f2b4ff5 into JuliaLang:master Oct 3, 2017
@bkamins bkamins deleted the patch-15 branch October 3, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants