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 String#byteindex and String#byterindex #2449

Merged

Conversation

choznerol
Copy link
Member

@choznerol choznerol commented Mar 21, 2023

Fix #2361

In this PR, byteindex and byterindex will become available for String just like index and rindex.

image

The incorrect output of index and rindex is a separate bug that is tracked by #2360.

Please let me know if equivalent amount of doctest is preferred for all four methods. I was just a bit of worry about being too verbose, so there isn't too many test case for some of them.

@choznerol
Copy link
Member Author

thread 'rustc' panicked at 'no resolutions for a doc link', compiler/rustc_metadata/src/rmeta/encoder.rs:2235:18

The current CI failure seems to be related to rust-lang/rust#109424

Copy link
Member

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

This change looks good, the functions do what I expect. Thanks for updating the doc tests to have multibyte UTF-8 characters.

I think you've got about the right amount of doc test examples.

Can you add real tests that are a bit more exhaustive? I'd like to see the following coverage:

  • all encodings for the receiver
  • None given as needle
  • Some([])
  • ASCII needles
  • Binary needles
  • UTF-8 needles with multibyte chars
  • testing on UTF-8 encoded strings that have binary contents.

Copy link
Member

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

this looks great.

Once rust-lang/rust#109500 is resolved I think the rustdoc build will be fixed and I'll poke CI to get this merged.

@choznerol
Copy link
Member Author

(Ops sorry I somehow didn't send out the comments yesterday 😅)

Can you add real tests that are a bit more exhaustive? I'd like to see the following coverage:

* all encodings for the receiver
* `None` given as needle
* `Some([])`
* ASCII needles
* Binary needles
* UTF-8 needles with multibyte chars
* testing on UTF-8 encoded strings that have binary contents.

Thanks for the detailed review! I've added the test only for byteindex in d9ce3c8, but not for byterindex to avoid duplication.

Copy link
Member

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

Thank you this is good to go. It looks like the rust doc folks are making progress on the nightly breakage. I'm following upstream and will poke the build here when nightly is fixed so we can get this merged.

@choznerol choznerol requested a review from lopopolo April 8, 2023 07:52
@lopopolo lopopolo added C-enhancement Category: New feature or request. A-ruby-core Area: Ruby Core types. labels Apr 10, 2023
@lopopolo
Copy link
Member

nightly rustdoc is fixed, the build is green, merging. thanks for the contribution @choznerol.

@lopopolo lopopolo merged commit 4d60cc0 into artichoke:trunk Apr 10, 2023
@choznerol choznerol deleted the 2361-add-String-byteindex-byterindex branch April 11, 2023 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ruby-core Area: Ruby Core types. C-enhancement Category: New feature or request.
Development

Successfully merging this pull request may close these issues.

Add byteindex and byterindex methods to spinoso-string
2 participants