Increase test coverage, coerce inputs to string and fix encoding inconsistencies #101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mudge
force-pushed
the
test-coverage
branch
2 times, most recently
from
279621d to
029e019
Compare
mudge
commented
Sep 15, 2023
mudge
force-pushed
the
test-coverage
branch
from
291bca1 to
ccb767d
Compare
mudge
commented
Sep 15, 2023
mudge
force-pushed
the
test-coverage
branch
from
ccb767d to
45f3ddb
Compare
mudge
changed the title
mudge
force-pushed
the
test-coverage
branch
from
af62a93 to
50d0700
Compare
Where possible, coerce inputs to strings with StringValue rather than raising a TypeError. This particularly impacts the relatively recent RE2::Set API which was excessively strict about its arguments. Add test coverage to all parts of the API, better covering edge cases including how encoding is handled based on the encoding of the given pattern.
When replacing with a string pattern, we will implicitly create an RE2 pattern from the string with the default options meaning it will assume and produce UTF-8 results. This could potentially be a breaking change for users who rely on the string pattern's encoding but the behaviour has been misleading (and passing anything except ISO-8859-1 or UTF-8 to RE2 is undefined).
As RE2::Regexp#match will only return true or false when given a pattern with no capturing groups (rather than returning an RE2::MatchData), document and test this edge case.
To distinguish "re2" the gem and "RE2" the C++ library, try to be consistent with capitalisation. Add a dedicated "Encoding" section to the README as well as adding the same documentation to every method that returns strings from RE2.
Follow sqlite3-ruby and Nokogiri's lead by including the licences of all vendored dependencies, see https://github.com/sparklemotion/sqlite3-ruby/blob/master/LICENSE-DEPENDENCIES and https://github.com/sparklemotion/nokogiri/blob/main/LICENSE-DEPENDENCIES.md
mudge
force-pushed
the
test-coverage
branch
from
e185c73 to
36942a9
Compare
mudge
commented
Sep 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Where possible, coerce inputs to strings with StringValue rather than raising a TypeError. This particularly impacts the relatively recent RE2::Set API which was excessively strict about its arguments.
Add test coverage to all parts of the API, better covering edge cases including how encoding is handled based on the encoding of the RE2 pattern.