TINKERPOP-3090 TrimGlobalStep handles unicode characters as Ltrim and Rtrim #2666
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.7-dev #2666 +/- ##
=============================================
+ Coverage 76.14% 76.71% +0.57%
- Complexity 13152 13187 +35
=============================================
Files 1084 1087 +3
Lines 65160 66368 +1208
Branches 7285 7299 +14
=============================================
+ Hits 49616 50916 +1300
+ Misses 12839 12740 -99
- Partials 2705 2712 +7 ☔ View full report in Codecov by Sentry. |
saikiranboga
approved these changes
Jun 26, 2024
spmallette
reviewed
Jun 26, 2024
CHANGELOG.asciidoc
Outdated
| @@ -39,6 +39,7 @@ This release also includes changes from <<release-3-6-7, 3.6.7>>. | |||
| * Deprecated `ltrim()` and `rTrim()` in favor of `l_trim()` and `r_trim` in Python. | |||
| * Fixed bug in `onCreate` for `mergeV()` where use of the `Cardinality` functions was not properly handled. | |||
| * Fixed multiple concurrent initially requests caused authentication to fail. | |||
| * Fixed so that TrimGlobalStep has the same character control handling as Ltrim and Rtrim | |||
There was a problem hiding this comment.
Looks like you need move your changelog entry up to 3.7.3
|
VOTE +1, pending CHANGELOG fix |
spmallette
changed the title
|
VOTE +1, also pending GLV feature test file updates (i.e. |
|
Good to also update |
vkagamlyk
reviewed
Jun 26, 2024
gremlin-test/src/main/resources/org/apache/tinkerpop/gremlin/test/features/map/RTrim.feature
Show resolved
Hide resolved
vkagamlyk
reviewed
Jun 26, 2024
CHANGELOG.asciidoc
Outdated
| @@ -30,6 +30,7 @@ This release also includes changes from <<release-3-6-8, 3.6.8>>. | |||
| * Deprecated public constructor for `SeedStrategy` in favor of builder pattern to be consistent with other strategies. | |||
| * Allow specifying a customized Spark app name | |||
| * CoinStep has a getter method for its probability field | |||
| * Fixed so that TrimGlobalStep has the same character control handling as Ltrim and Rtrim | |||
There was a problem hiding this comment.
nit: mention TrimLocalStep
|
VOTE+1 |
done! Now they all rely on the same common method |
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.
https://issues.apache.org/jira/browse/TINKERPOP-3090
The current behavior is inconsistent because it relies upon a different API to detect space characters. I think String#trim does not handle unicode characters.