Skip to content

Add missing overloads for String#byte_slice#12809

Merged
straight-shoota merged 5 commits intocrystal-lang:masterfrom
straight-shoota:feature/string-byte_slice-range
Dec 23, 2022
Merged

Add missing overloads for String#byte_slice#12809
straight-shoota merged 5 commits intocrystal-lang:masterfrom
straight-shoota:feature/string-byte_slice-range

Conversation

@straight-shoota
Copy link
Member

String#byte_slice and #byte_slice have signatures accepting a start index plus count. String#byte_slice also has an overload with only a start index.

There are no overloads accepting a Range argument, which is typically present in similar methods (most prominently Indexable#[]).

This patch adds:

  • String#byte_slice(range : Range) - resolves range indices and delegates to #byte_slice(start : Int, count : Int)
  • String#byte_slice?(range : Range) - resolves range indices and delegates to #byte_slice?(start : Int, count : Int)
  • String#byte_slice?(start : Int) - counterpart to String#byte_slice(start : Int)

@@ -1173,9 +1173,9 @@ class String
# "hello".byte_slice(-2, 5) # => "he"
Copy link
Contributor

@HertzDevil HertzDevil Dec 2, 2022

Choose a reason for hiding this comment

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

This and the two calls above actually return "lo". I don't know why these aren't caught by the routine "fix examples in the docs" PRs

(those lines are due to #8447 so they are fairly old)

Copy link
Member Author

Choose a reason for hiding this comment

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

@maiha Any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

The basic premise is that my checkers cannot handle all EXAMPLES.

EXAMPLES that cannot be handled are managed as an "exclusion list" and excluded from testing.

In this case, the entire code block is excluded because the incorrect comment syntax would interfere with the automatic generation of tests.

"¥hello".byte_slice(0, 1)  # => "\C2" (invalid UTF-8 character)
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^

The following code is generated and fails.

$ cd tmp/string/working && LC_ALL=C /v/crystal/bin/crystal run -D preview_overflow ../all_spec.cr 2>&1
Using compiled compiler at /v/crystal/.build/crystal
In /v/tmp/string/all_spec.cr:509:46

 509 | �[0m�[1m( "¥hello".byte_slice(0, 1) ).should eq( "�" (invalid UTF-8 character) )
                                                    ^
Error: expecting token ')', not '('

I understand. So how many examples of such exclusions do you have?

1979 test
1341 success 43 pending 3 general 147 pseudo 71 macro 9 wrong 44 random 321 failure

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we could fix a format for such comments on the output value.

My initial idea for this would be to just append another comment:

"¥hello".byte_slice(0, 1)  # => "\C2" # invalid UTF-8 character

This should be fairly readable, unambiguous and easy to parse.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's good. I think it is the best choice so far. 💯

Copy link
Contributor

Choose a reason for hiding this comment

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

@straight-shoota
I would like to inspect the excluded EXAMPLES like this one once again.

There are probably about 300 cases, and it takes about a week.
I smell a 1.7 release, will it be ready in time?

Copy link
Member Author

Choose a reason for hiding this comment

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

There should be time. 1.7.0 is scheduled for January 6.

straight-shoota and others added 2 commits December 2, 2022 23:34
Co-authored-by: Quinton Miller <nicetas.c@gmail.com>
@straight-shoota
Copy link
Member Author

@HertzDevil Thanks for digging into the examples. I know there were some issues with that and meant to look into using https://github.com/maiha/crystal-examples for checking. But I didn't get anywhere and lost track before publishing the PR.

@straight-shoota straight-shoota self-assigned this Dec 8, 2022
@straight-shoota straight-shoota added this to the 1.7.0 milestone Dec 22, 2022
@straight-shoota straight-shoota merged commit d3c68ec into crystal-lang:master Dec 23, 2022
@straight-shoota straight-shoota deleted the feature/string-byte_slice-range branch December 23, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants