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

string.c: Add String#delete_prefix to remove leading substr #1632

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

6 participants
@sonots
Member

sonots commented May 25, 2017

@sonots sonots changed the title from string.c: Add String#remove_prefix to remove leading substr to string.c: Add String#delete_prefix to remove leading substr Jun 16, 2017

@sonots sonots requested a review from nobu Jun 16, 2017

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 16, 2017

Member

I want to get reviewed before merging. @nobu could you review my codes?

Member

sonots commented Jun 16, 2017

I want to get reviewed before merging. @nobu could you review my codes?

Show outdated Hide outdated string.c
@knu

Nice addition! Do you plan to propose String#delete_suffix as well?

Show outdated Hide outdated string.c
@nobu

This comment has been minimized.

Show comment
Hide comment
@nobu

nobu Jun 16, 2017

Member
$ ./ruby -v -e 'class NilClass; alias to_str to_s; end' -e '"abc".delete_prefix(nil)'
ruby 2.5.0dev (2017-06-16 trunk 59100) [x86_64-darwin15]
last_commit=string.c: add String#delete_prefix and String#delete_prefix! to remove leading substr [Feature #12694]
-e:2: [BUG] Segmentation fault at 0x0000000000000008

rb_str_start_with converts arguments to String, you should reuse the string in these methods.

Member

nobu commented Jun 16, 2017

$ ./ruby -v -e 'class NilClass; alias to_str to_s; end' -e '"abc".delete_prefix(nil)'
ruby 2.5.0dev (2017-06-16 trunk 59100) [x86_64-darwin15]
last_commit=string.c: add String#delete_prefix and String#delete_prefix! to remove leading substr [Feature #12694]
-e:2: [BUG] Segmentation fault at 0x0000000000000008

rb_str_start_with converts arguments to String, you should reuse the string in these methods.

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 16, 2017

Member

@knu String#delete_suffix can be achieved by String#chomp. But, do you think we should have it for symmetry with delete_prefix? I am neutral on this.

Member

sonots commented Jun 16, 2017

@knu String#delete_suffix can be achieved by String#chomp. But, do you think we should have it for symmetry with delete_prefix? I am neutral on this.

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 16, 2017

Member

Thank you for reviews. I will try to fix.

Member

sonots commented Jun 16, 2017

Thank you for reviews. I will try to fix.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Jun 16, 2017

Member

Yeah, so the names really matter in Ruby and we need some research for compelling reasons to pick particular names. What do other languages and libraries say about the names for those operations?

Member

knu commented Jun 16, 2017

Yeah, so the names really matter in Ruby and we need some research for compelling reasons to pick particular names. What do other languages and libraries say about the names for those operations?

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 16, 2017

Member

I've researched other languages at https://bugs.ruby-lang.org/issues/12694#note-12, and golang has similar methods named TrimPrefix and TrimSuffix.
Because matz decided the name as delete_prefix https://bugs.ruby-lang.org/issues/12694#note-18, I think the suffix version should become delete_suffix.
My concern is whether we should add the suffix version although we can do similar operations with String#chomp. So, you think we should add it? I can submit to bugs.ruby-lang.org.

Member

sonots commented Jun 16, 2017

I've researched other languages at https://bugs.ruby-lang.org/issues/12694#note-12, and golang has similar methods named TrimPrefix and TrimSuffix.
Because matz decided the name as delete_prefix https://bugs.ruby-lang.org/issues/12694#note-18, I think the suffix version should become delete_suffix.
My concern is whether we should add the suffix version although we can do similar operations with String#chomp. So, you think we should add it? I can submit to bugs.ruby-lang.org.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Jun 16, 2017

Member

Aww my bad, I wasn't following that. Never mind, better discussed there.

Member

knu commented Jun 16, 2017

Aww my bad, I wasn't following that. Never mind, better discussed there.

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 16, 2017

Member

@rhenium @nobu Fixed for argument of a broken string, and of non-string object. Could you review again?

Member

sonots commented Jun 16, 2017

@rhenium @nobu Fixed for argument of a broken string, and of non-string object. Could you review again?

@nobu

nobu approved these changes Jun 16, 2017

Show outdated Hide outdated string.c
Show outdated Hide outdated string.c
Show outdated Hide outdated string.c
@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 16, 2017

Member

@nobu thank you for reviews. fixed.

Member

sonots commented Jun 16, 2017

@nobu thank you for reviews. fixed.

Show outdated Hide outdated string.c
@nobu

This comment has been minimized.

Show comment
Hide comment
@nobu

nobu Jun 18, 2017

Member

I've fixed a bug in chomp! at f5052d4, and this delete_prefix! has same bug too.

Member

nobu commented Jun 18, 2017

I've fixed a bug in chomp! at f5052d4, and this delete_prefix! has same bug too.

@nobu

This comment has been minimized.

Show comment
Hide comment
@nobu

nobu Jun 18, 2017

Member

Sorry, delete_prefix! raises a RuntimError properly, as it uses rb_str_drop_bytes.
Just str_modify_keep_cr(str) can be replaced with str_modifiable(str) not to make shared string unnecessarily.

Member

nobu commented Jun 18, 2017

Sorry, delete_prefix! raises a RuntimError properly, as it uses rb_str_drop_bytes.
Just str_modify_keep_cr(str) can be replaced with str_modifiable(str) not to make shared string unnecessarily.

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 18, 2017

Member

Thank you for letting me know. I've added same tests for test_delete_prefix_bang anyway.

Member

sonots commented Jun 18, 2017

Thank you for letting me know. I've added same tests for test_delete_prefix_bang anyway.

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 19, 2017

Member

If there is no more comment for a day, I will squash and merge into trunk.

Member

sonots commented Jun 19, 2017

If there is no more comment for a day, I will squash and merge into trunk.

s = S("abba")
assert_equal("bba", s.delete_prefix(klass.new))
assert_equal("abba", s)
end

This comment has been minimized.

@eregon

eregon Jun 19, 2017

Member

Just as a suggestion, it would be very useful to have this in form of separate ruby/spec examples under spec/rubyspec.
Small separate spec examples allows to develop the feature on other Ruby implementations incrementally and in case one specific example is hard to implement still ensures the rest keeps working as intended.
It also documents the functionality explicitly, which appears in the test output and not just in comments in the code.
If you are unfamiliar with the spec style or ruby/spec, I'm happy to help.
I'm chiming in because, as an alternative Ruby implementer, I (and I believe most other alternative Ruby implementers) find it very valuable to have these multiple small specs with descriptions over just a big test/unit method, even more so for new features.
But again, this is just a suggestion.

@eregon

eregon Jun 19, 2017

Member

Just as a suggestion, it would be very useful to have this in form of separate ruby/spec examples under spec/rubyspec.
Small separate spec examples allows to develop the feature on other Ruby implementations incrementally and in case one specific example is hard to implement still ensures the rest keeps working as intended.
It also documents the functionality explicitly, which appears in the test output and not just in comments in the code.
If you are unfamiliar with the spec style or ruby/spec, I'm happy to help.
I'm chiming in because, as an alternative Ruby implementer, I (and I believe most other alternative Ruby implementers) find it very valuable to have these multiple small specs with descriptions over just a big test/unit method, even more so for new features.
But again, this is just a suggestion.

This comment has been minimized.

@sonots

sonots Jun 19, 2017

Member

I talked (actually, chat) with eregon. I personally do not mind writing tests on either test/ or spec/, but I do not want to write same tests twice. This is a ticket to discuss about this problem https://bugs.ruby-lang.org/issues/13666.

I will merge my PR without waiting the conclusion of the test/ or spec/ discussion. Backporting test/ into spec/ is welcome for now.

@sonots

sonots Jun 19, 2017

Member

I talked (actually, chat) with eregon. I personally do not mind writing tests on either test/ or spec/, but I do not want to write same tests twice. This is a ticket to discuss about this problem https://bugs.ruby-lang.org/issues/13666.

I will merge my PR without waiting the conclusion of the test/ or spec/ discussion. Backporting test/ into spec/ is welcome for now.

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Jun 19, 2017

Member

Please also add a entry to the NEWS file when you commit this.

Member

eregon commented Jun 19, 2017

Please also add a entry to the NEWS file when you commit this.

@sonots

This comment has been minimized.

Show comment
Hide comment
@sonots

sonots Jun 19, 2017

Member

I will make NEWS before merging since it often conflicts...

Member

sonots commented Jun 19, 2017

I will make NEWS before merging since it often conflicts...

sonots added a commit to sonots/ruby that referenced this pull request Jun 19, 2017

string.c: add String#delete_prefix and String#delete_prefix!
to remove leading substr [Feature #12694] [rubyGH-1632]

* string.c (rb_str_delete_prefix_bang): add a new method
  to remove prefix destuctively.

* string.c (rb_str_delete_prefix): add a new method
  to remove prefix non-destuctively.

* test/ruby/test_string.rb: add tests.

sonots added a commit to sonots/ruby that referenced this pull request Jun 19, 2017

string.c: add String#delete_prefix and String#delete_prefix!
to remove leading substr [Feature #12694] [fix rubyGH-1632]

* string.c (rb_str_delete_prefix_bang): add a new method
  to remove prefix destuctively.

* string.c (rb_str_delete_prefix): add a new method
  to remove prefix non-destuctively.

* test/ruby/test_string.rb: add tests.

sonots added a commit to sonots/ruby that referenced this pull request Jun 21, 2017

string.c: add String#delete_prefix and String#delete_prefix!
to remove leading substr [Feature #12694] [fix rubyGH-1632]

* string.c (rb_str_delete_prefix_bang): add a new method
  to remove prefix destuctively.

* string.c (rb_str_delete_prefix): add a new method
  to remove prefix non-destuctively.

* test/ruby/test_string.rb: add tests.

sonots added some commits May 25, 2017

string.c: add String#delete_prefix and String#delete_prefix!
to remove leading substr [Feature #12694] [fix GH-1632]

* string.c (rb_str_delete_prefix_bang): add a new method
  to remove prefix destuctively.

* string.c (rb_str_delete_prefix): add a new method
  to remove prefix non-destuctively.

* test/ruby/test_string.rb: add tests.
test/ruby/test_string.rb: add tests to chomp substr
* test/ruby/test_string.rb (TestString#test_chomp): add tests
  to chomp substr

* test/ruby/test_string.rb (TestString#test_chomp!): ditto

@matzbot matzbot closed this in 1008236 Jun 21, 2017

@sonots sonots deleted the sonots:remove_prefix branch Jun 21, 2017

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