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 head and tail arguments to chop #24126

Merged
merged 5 commits into from
Oct 19, 2017
Merged

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 13, 2017

This PR implements the second part of the proposal #23765.

if head == 0
tail == 0 && return SubString(s)
return SubString(s, 1, prevind(s, endof(s), tail))
end
Copy link
Member

Choose a reason for hiding this comment

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

If would find it more readable with an else block and a return so that the two cases are completely parallel.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

julia> chop(a, 1, 2)
"ar"

julia> chop(a, 5, 5)
Copy link
Member

Choose a reason for hiding this comment

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

The description should mention that no error is thrown if there are not enough characters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added. This is the current behavior as chop("") returns "".

# negative values of head/tail will throw error in nextind/prevind
if head == 0
tail == 0 && return SubString(s)
return SubString(s, 1, prevind(s, endof(s), tail))
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK we use start(s) instead of 1 in the existing code, though I suspect we should move to 1 everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to start. There is a mixed style in the code base (I have seen both during the last month :)), but I agree that it is better to use start. I fix some more situations like this in strings/util.jl.

NEWS.md Outdated
@@ -249,6 +249,9 @@ This section lists changes that do not have deprecation warnings.
Library improvements
--------------------

* The function `chop` now accepts two arguments `head` and `tail` allowing to specify
number of characters to remove from the head and tail tail of the string ([#24126]).
Copy link
Member

Choose a reason for hiding this comment

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

"tail tail"? :-)

# negative values of head/tail will throw error in nextind/prevind
if head == 0
if tail == 0
return SubString(s)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be simpler to compute the start and end indices in the branches, and call SubString only once at the end? SubString will call endof anyway if you don't do it yourself. That should make the function body smaller and allow inlining when appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

an excellent suggestion. Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, indeed that's compact!


Remove the last character from `s`.
Remove first `head` and last `tail` characters from `s`.
Copy link
Member

Choose a reason for hiding this comment

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

I'd have said "the first" and "the last". It would also make sense to continue saying what the default does for clarity, since that's a standard function.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -94,10 +118,10 @@ julia> chomp("Hello\\n")
"""
function chomp(s::AbstractString)
i = endof(s)
(i < 1 || s[i] != '\n') && (return SubString(s, 1, i))
(i < start(s) || s[i] != '\n') && (return SubString(s, start(s), i))
Copy link
Member

Choose a reason for hiding this comment

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

Better leave this for another PR as it's unrelated. Anyway it's not worth changing these if we don't do it everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@kshyatt kshyatt added the strings "Strings!" label Oct 16, 2017
@bkamins
Copy link
Member Author

bkamins commented Oct 16, 2017

rebased

@ararslan ararslan merged commit 9d3b6c7 into JuliaLang:master Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants